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

Dialogs #70

Merged
merged 11 commits into from
Apr 5, 2024
Merged

Dialogs #70

merged 11 commits into from
Apr 5, 2024

Conversation

borjaMarti
Copy link
Collaborator

@borjaMarti borjaMarti commented Apr 3, 2024

Description

This PR normalizes adds accessible dialogs using the native HTML5 element to prompt confirmation from the user when deleting a list or an item.

It also includes some minor bug-fixes, such as the spinner background color, the Layout/main element height, and the hover behavior of the buttons on the List and ManageList views.

Related Issue

closes #69

Acceptance Criteria

  • When prompting the user for confirmation, a dialog should be used.
  • The dialog should adhere to accessibility guidelines, and be fully controllable via keyboard.

Type of Changes

Type
✨ New feature
🎨 Enhancement
🐛 Bug fix

Updates

Before

image

After

image

image

image

Testing Steps / QA Criteria

  • git pull
  • git checkout dialogs
  • Check behavior when deleting lists and items.

Copy link

github-actions bot commented Apr 3, 2024

Visit the preview URL for this PR (updated for commit c4d40b5):

https://tcl-71-smart-shopping-list--pr70-dialogs-v086fkf9.web.app

(expires Fri, 12 Apr 2024 08:29:18 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1e7ade9d0f374c4ddb5d7ab6fc541062fc7a1ab4

Copy link
Collaborator

@ocsiddisco ocsiddisco left a comment

Choose a reason for hiding this comment

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

Thanks for working on this Borja! I tested on dekstop and mobile and it works as expected.
My only concern here is the UI. You added a primary button Cancel (lightPurple) and a secondary one Confirm. This is correct to guide the user, in our case the colors of the buttons should be inversed, the primary button is the Confirm and should have lightPurple color, and the secondary Cancel with the bg-puurwhite.
Let me know if you want to proceed with the changes and if you need any help.

@borjaMarti
Copy link
Collaborator Author

Thanks for working on this Borja! I tested on dekstop and mobile and it works as expected. My only concern here is the UI. You added a primary button Cancel (lightPurple) and a secondary one Confirm. This is correct to guide the user, in our case the colors of the buttons should be inversed, the primary button is the Confirm and should have lightPurple color, and the secondary Cancel with the bg-puurwhite. Let me know if you want to proceed with the changes and if you need any help.

Hey @ocsiddisco! I went with the primary palette on Cancel because while doing an article on dialogs when I first implemented them, there was a mention about the "least destructive" option being the default one (so, for example, when opening the dialog with a keyboard, Cancel should be the focused option), which I extended to the palette, as well as the color choice on confirm (alertRed indicating it's a non-reversible action.

But it makes sense that if the user is going to delete something, visually the primary action is continuing the action, and the alertRed color looks a bit out of place in contrast to the rest of the app, so I went ahead and implemented your suggestion!

@ocsiddisco
Copy link
Collaborator

Thanks for the information @borjaMarti and the update! It does seem more intuitive after the update.
Should you still prefer implementing a red alert for the user, I would suggest to update the background of the primary button, to the AlertRed with an opacity 0.8:
image

I used this article to dubble check the UI:
https://medium.com/@joaopegb/ux-writing-an-effective-cancel-dialog-confirmation-on-web-539b73a39929

import Confirm from './Confirm';

const DeleteItem = ({ itemName, listPath, itemId }) => {
const [isConfirmOpen, setIsConfirmOpen] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you rename IsConfirmed field? Is prefix is not suggested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!


const DeleteItem = ({ itemName, listPath, itemId }) => {
const [isConfirmOpen, setIsConfirmOpen] = useState(false);
const [isSubmitted, setIsSubmitted] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as IsConfirmed

@borjaMarti borjaMarti requested a review from elitcenk April 4, 2024 10:26
@borjaMarti borjaMarti merged commit 4ae38e0 into main Apr 5, 2024
1 check failed
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.

30. Dialogs instead of confirm()
5 participants