-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add confirm discard page as an extra step when discarding a draft (un… #2134
Conversation
…published) manual Replace the existing system alert popup with a separate page to provide a better user experience.
6339f51
to
daa5f67
Compare
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'm approving as it has test coverage and the code looks good. I'll leave it up to you about the test mix - I do wonder whether this should have a controller spec as well/instead of a feature test
features/deleting-a-manual.feature
Outdated
And I am on the show page for that manual | ||
Then I should not see the discard draft button | ||
And I visit the confirm discard page directly | ||
Then I am on the show page for that manual |
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.
Feels like this should perhaps be two different scenarios? One testing the button visibility and another the direct access attempt? I might even go further and question whether a lower-level test might be appropriate here as feature tests are quite expensive (assuming they are running via a browser), so I tend to prefer them for happy path only tests. But that may not be practical here and obviously that's a very subjective point
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 tend to agree with everything you've said, there just aren't much in the way of controller/view specs in the manuals publisher codebase. However, we have taken a stab at introducing some, and removed the cucumber test. Let us know your thoughts! 😄
Replace the feature tests around the ability to discard published manuals with lower-level controller and view specs (which will be quicker to run).
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.
Thanks for reviewing the tests, they look nicely balanced now to me
…published) manual
What
When discarding a draft (unpublished) manual, replace the existing confirmation system alert popup with a separate confirmation page.
Why
To improve user experience and move towards the design system.
Visuals
Clicking the Discard draft manual button
Would raise this popup
That the user would then click OK on to confirm discard.
With the changes in this PR, it will instead take the user to a confirmation page:
Where clicking Discard draft will actually perform the discard operation (as clicking OK on the previous alert).
Once discarded, the success message, which was appearing as an info message:
Will now appear styled as a success message:
Trello card
Follow these steps if you are doing a Rails upgrade.