-
Notifications
You must be signed in to change notification settings - Fork 537
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
Modify Button Behaviour When enable-submissions Flag is Disabled #22779
Modify Button Behaviour When enable-submissions Flag is Disabled #22779
Conversation
@chrstinalin could you reassign review. I'll be on PTO next week and don't want to block you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing some context here, but why are we doing the disabling, and rendering the error, in javascript? We know submissions are disabled when the template is rendered so add the logic to disable the buttons and display the message in the template?
We can test the error message is visible and the button disabled in tests too that way
I honestly can't recall if I had a reason for this, I think it may have been the natural progression of my original barebones fix... That solution makes alot more sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits in the test code, and it works fine. I want to get a second opinion on the UI though - exposing the reason in a tooltip you have to mouse-over to seee feels too hidden to me. Can you include a screenshot of what it looks like for reference? (you've already included one)
I'm no UX/Product person but I think it should be a bit more obvious. My idea was to prevent submissions while at the same time still showing a page that looks somewhat normal, because the error page you otherwise get from the decorator is a bit too generic/empty (it's good as a fallback for anyone trying to bypass the more user-friendly error). |
@diox My initial fear with directly putting the reason on the page is if the reason is longer than usual (which may not happen) and possibly look weird. Though IIRC there's somewhere in DevHub that use a banner-style information section, which might work better? I can't seem to find it, though. |
Search for |
Fixes: mozilla/addons#15090
Description
when
enable-submissions
is disabled, now:Testing
enable-submissions
waffle flag (everyone should be =True
by default).Checklist
#ISSUENUM
at the top of your PR to an existing open issue in the mozilla/addons repository.