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 confirm alerts show the triggering action #4543

Merged

Conversation

rhian-cs
Copy link
Contributor

@rhian-cs rhian-cs commented Jun 10, 2021

References

Related Issues/Pull Requests/errors/etc...

Resolves #4015

Objectives

What are the objectives of these changes?

When the user clicks on certain buttons in the app, a confirmation alert is shown. However, this alert only asks the user "Are you sure?", but that doesn't give much information about what he should be sure of.

In this PR, I made the confirmation alerts show the triggering action, with the format:

Are you sure? %{action}

For instance, if the user clicks on a 'Hide author' button, the alert now displays 'Hide author: Are you sure?'.

System specs were also updated to check if the confirmation alerts actually showed the triggering action.

Visual Changes

Any visual changes? please attach screenshots (or gifs) showing them.
If modified views are public (not the admin panel), try them in mobile display (with your browser's developer console) and add screenshots.

For example, in a page that has comments:

01-confirmation-screenshot

I didn't include a mobile screenshot because the dev tools still show the alerts as if it was in a computer, but since this is the standard alert, not much has changed.

Notes

Mention rake tasks or actions to be done when deploying this changes to a server (if any).
Explain any caveats, or important things to notice like deprecations (if any).

Since this PR is already pretty big, I plan to open another one (if/after this one is merged) to update the other locales too.

@rhian-cs rhian-cs force-pushed the 4015-detail-confirmation-alerts branch from 23f3d79 to c4d0485 Compare June 29, 2021 12:47
@javierm javierm added this to Reviewing in Consul Democracy via automation Oct 17, 2021
@javierm javierm added UX and removed post-1.4 labels Nov 2, 2021
@javierm
Copy link
Member

javierm commented Nov 12, 2021

Hi, @rhian-cs 😄.

Thank you so much for this pull request, and sorry for the late reply! 🙏.

As part of a list of accessibility improvements regarding the links and buttons in the admin section, we implemented the detailed confirmation dialog in the admin section so it includes the name of the thing it acts on. The general message format is: "Are you sure? %{action} \"%{name}\"", so for example it would read like Are you sure? Hide "Send an expedition to the Sun" for a proposal titled "Send an expedition to the Sun". See commit 6510cb9.

Would it be possible to rebase this pull request against the latest master branch and update it so it uses that format in the public sections? If you can, great! 🎉. If you can't, it's OK; we understand we've kept you waiting for a review far too long.

Thanks!

@javierm javierm moved this from Reviewing to Doing in Consul Democracy Nov 12, 2021
@rhian-cs
Copy link
Contributor Author

Hello @javierm! I'll take another look, this PR was pretty big too.

I hope it doesn't take too long since this branch is pretty outdated as well.

We could perhaps try to redo it or split it into smaller PRs, WDYT?

@javierm
Copy link
Member

javierm commented Nov 12, 2021

@rhian-cs Thanks! 😄 One pull request is OK, if that's fine with you.

Regarding whether it's better to redo it or not, whatever suits you better works for us 😉.

@javierm javierm closed this Nov 12, 2021
@javierm javierm reopened this Nov 12, 2021
@javierm
Copy link
Member

javierm commented Nov 12, 2021

(I clicked on "close with comment" instead of "comment" by accident; please ignore that 😌)

@rhian-cs rhian-cs force-pushed the 4015-detail-confirmation-alerts branch 4 times, most recently from ebf038f to e229347 Compare December 15, 2021 15:16
@rhian-cs
Copy link
Contributor Author

Hello @javierm!

I've rebased the branch and fixed the failing specs. I also ran all the specs locally. Could you take a look at this and let the CI run the tests?

@javierm javierm moved this from Doing to Reviewing in Consul Democracy Dec 15, 2021
@javierm
Copy link
Member

javierm commented Dec 15, 2021

Hi, @rhian-cs 😄.

This is great, thanks! We'll have a look at this pull request before the end of the year 👍.

Oh, and thank you so much for taking the time to update the tests 😉.

Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

@rhian-cs Thank you so much, this is great! 😄 🎉

I've left a few comments; let me know what you think! And, by all means, if you don't agree with something I mention, say so! 😉

config/locales/en/moderation.yml Show resolved Hide resolved
app/views/proposal_notifications/_actions.html.erb Outdated Show resolved Hide resolved
spec/system/admin/banners_spec.rb Outdated Show resolved Hide resolved
spec/system/admin/budget_groups_spec.rb Outdated Show resolved Hide resolved
spec/system/admin/budget_groups_spec.rb Outdated Show resolved Hide resolved
spec/system/admin/geozones_spec.rb Outdated Show resolved Hide resolved
spec/system/admin/geozones_spec.rb Outdated Show resolved Hide resolved
spec/system/admin/valuator_groups_spec.rb Outdated Show resolved Hide resolved
@javierm javierm moved this from Reviewing to Doing in Consul Democracy Dec 21, 2021
@javierm javierm changed the title [#4015] Make confirm alerts show the triggering action Make confirm alerts show the triggering action Dec 21, 2021
@javierm javierm moved this from Doing to Reviewing in Consul Democracy Dec 22, 2021
@javierm javierm force-pushed the 4015-detail-confirmation-alerts branch 2 times, most recently from 8e33745 to 609e58c Compare December 22, 2021 11:55
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

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

Thanks! 👍 Really appreciate it 😄.

Consul Democracy automation moved this from Reviewing to Testing Dec 22, 2021
@javierm javierm merged commit 697c1a4 into consuldemocracy:master Dec 22, 2021
Consul Democracy automation moved this from Testing to Release 1.5.0 Dec 22, 2021
@rhian-cs rhian-cs deleted the 4015-detail-confirmation-alerts branch December 22, 2021 12:21
@javierm javierm self-assigned this Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detailed confirmation texts
2 participants