Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widget state persistance #4574

Merged
merged 19 commits into from
Jul 31, 2024
Merged

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Jul 16, 2024

SDBELGA-847

TODO:

  • Look into possible different solutions for closedThroughAction
  • Add comment for the functionality
  • Look into state getting reset, when:
    a) widget is pinned and you click to close it
    b) widget is pinned or not, you switch back and forth between views, and switch widgets
  • Final check against develop
  • Update UIF so disabled take effect (version)
  • Add a playwright test
  • Verify persistence works across all widgets in React Authoring

What this PR aims to address: When a user switches back and forth between full view width and normal width, if there's a pinned widget that has state changes, the changes should persist between the two view modes. In this PR that's being handled using a HOC (WidgetStatePersistanceHOC) and a variable (closedThroughAction). The HOC handles the logic for state persistence using widgetState variable and ref (to get child component state - current widget). Inside authoring when a widget gets loaded we handle the initialState of the widget using closedThroughAction variable - If a widget has been closed by the user on purpose, once the same widget is reopened, state does get reset. If that's not the case (switching between the two article view modes) we count that as the widget not being closed through a direct user interaction, thus retrieving the old state of the widget and loading it with the widget rendering.

Side issues this PR fixes: Widget pinned state in the different scenarios, and widget persistence using preferencesService.

Additional solutions: Another solution for setTimeout for the initialState in authoring-react - Add a variable renderCount and based on the count delete the localStorage value, then reset renderCount. Problems with that - adds more complexity, if logic changes we'd need to adjust that on top of everything.

Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add a playwright test
  • Add documentation as a comment somewhere describing the issue and your solution

scripts/appConfig.ts Outdated Show resolved Hide resolved
scripts/core/superdesk-api.d.ts Outdated Show resolved Hide resolved
scripts/apps/authoring-react/authoring-react.tsx Outdated Show resolved Hide resolved
scripts/apps/authoring/widgets/widgets.ts Outdated Show resolved Hide resolved
scripts/apps/authoring-react/widget-persistance-hoc.tsx Outdated Show resolved Hide resolved
scripts/apps/dashboard/widget-react.tsx Outdated Show resolved Hide resolved
scripts/apps/authoring-react/widget-persistance-hoc.tsx Outdated Show resolved Hide resolved
Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take a look at 2 remaining unresolved comments

@thecalcc
Copy link
Contributor Author

"Versions & History" and "Comments" widget are not the usual implementations of widgets.

Nested components with multiple levels of state management, and more. I'd say time wasted is not worth it now, we can make a ticket to check it when working on authoring-react.

@thecalcc
Copy link
Contributor Author

Lint failing because UIF isn't yet updated

scripts/core/superdesk-api.d.ts Show resolved Hide resolved
scripts/core/superdesk-api.d.ts Show resolved Hide resolved
/**
* Used for controlling when a widget is closed intentionally by a user action
*/
export let closedOnRender = {closed: false};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the object is only neccessary for mutation to work, it would be better to call the object property "value", because the real name is already used in the constant.

I don't think closedOnRender is a good name either. What does it have to do with rendering? closedIntentionally sounds better. Also comment on how it may get closed unitentionally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also comment on how it may get closed unitentionally.

scripts/apps/authoring-react/widget-persistance-hoc.tsx Outdated Show resolved Hide resolved
scripts/extensions/ai-widget/src/ai-assistant.tsx Outdated Show resolved Hide resolved
scripts/apps/dashboard/widget-react.tsx Outdated Show resolved Hide resolved
@thecalcc thecalcc merged commit 49ef9c2 into superdesk:develop Jul 31, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants