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

Make preferences page editable without clicking 'Edit Preferences' #5449

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

When you select My Preferences from the user dropdown, you get to /preferences, which is a page that shows you your preferences but doesn't let you edit them. If you want to edit, you'll have to make one extra click on the Edit Preferences button. See #5324 (comment) for some click count considerations.

The question is why do you need the uneditable preferences page. It was added in 2403630. The reasoning according to #3167 was to split the settings form. But that doesn't explain why the uneditable page was added. When you click My Settings, you get to an editable page. When you click My Preferences, you don't. If you don't want to edit your preferences, can you just view them on the editable page? I think you can. Why don't we make My Preferences always editable?

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

The idea is fine but I think we should probably keep the edit action and get rid of the show action instead of moving edit to show? That's how the settings page works...

@Starmobilerepair

This comment was marked as spam.

@AntonKhorev
Copy link
Collaborator Author

AntonKhorev commented Dec 30, 2024

Keeping edit is not without its problems.

Open My Settings page. Your location is going to be /account/edit. Make an invalid edit, for example, enter a password and leave Confirm Password blank. Click Save Changes. You'll see the form loaded again with errors highlighted. But look at your location: now it's /account, a page that doesn't exist. Press Reload in the browser and you'll get 404. Are you sure you want that for preferences too?

I guess we can add a redirect from /preferences to /preferences/edit. Submitting from /preferences/edit will change the location to /preferences, reloading it will redirect back to /preferences/edit. But is it really better?

I think that /account/edit has mostly to do with the page advertised as "Settings" yet the controller/path being "Account". It's not like that in case of Preferences.

@gravitystorm
Copy link
Collaborator

One option would be to keep the #show and #edit pages as they are now, but to link directly to the #edit page from the menu. That way, if the user presses refresh after a failed update, they will see the #show page.

@AntonKhorev
Copy link
Collaborator Author

Does anyone need the #show page?

@gravitystorm
Copy link
Collaborator

Does anyone need the #show page?

No, I guess not.

I've done a small amount of research into this, to see what other similar applications do - just to see if there's any conventions. Github, Mastodon and Discourse all show forms and don't appear to have a show/edit split. Mastodon has its source code available, and they use show+update actions, with the form on the show page. This avoids the whole issue around the path changing when forms are redisplayed and therefore breaking refresh.

https://github.com/mastodon/mastodon/tree/3c7f3b190cbaf6b6db25b15c16c8dc0bff599c00/app/controllers/settings/preferences (they've split their preferences into multiple pages, but BaseController#update shows the logic).

So maybe our convention should be that where it isn't useful to have separate show and edit pages, then we put the form on the show page.

I'd be happy to hear from anyone who has advice based on other similar rails projects!

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.

4 participants