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

Fix #4025: add user setting to keep the view mode. #4027

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

zyhfish
Copy link
Contributor

@zyhfish zyhfish commented Mar 20, 2024

No description provided.

@sbwalker
Copy link
Member

Interactive render mode supports the ability to stay in Edit Mode by utilizing PageState - which is scoped to the user session. Static render mode is stateless and therefore needs an alternative for maintaining this information.

In reviewing this PR, it appears to be using both cookies and UserSettings. In general I am not in favor of using cookies - especially the way this is implemented where it forces a new cookie for both authenticated and non-authenticated users. EditMode is only applicable to authenticated users so the state should only be managed for that scenario.

It seems like this could be simplified...

  • Based on a single UserSetting item named "EditModePageId"
  • When a user clicks the button to toggle Edit Mode it will either set the UserSetting value to PageState.Page.PageId or ""
  • The SiteRouter can reference PageState.User.Settings("EditModePageId") and if it matches PageState.Page.PageId then set PageState.EditMode accordingly, and if it does not match then set the UserSetting value to ""

@zyhfish
Copy link
Contributor Author

zyhfish commented Mar 21, 2024

Hi @sbwalker , thx so much for the review, code is updated.

@sbwalker
Copy link
Member

sbwalker commented Mar 21, 2024

@zyhfish this PR is still calling SettingService.GetUserSettingsAsync(user.UserId) in the SiteRouter which will affect performance and should be using user.Settings() since it already exists. I can merge this PR and make some final adjustments.

@sbwalker sbwalker merged commit 8adbdcc into oqtane:dev Mar 21, 2024
1 check passed
sbwalker added a commit to sbwalker/oqtane.framework that referenced this pull request Mar 21, 2024
sbwalker added a commit that referenced this pull request Mar 21, 2024
@zyhfish zyhfish deleted the task/fix-issue-4025 branch March 22, 2024 00:31
@zyhfish
Copy link
Contributor Author

zyhfish commented Mar 22, 2024

Hi @sbwalker , thx so much for the refactoring.

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