-
Notifications
You must be signed in to change notification settings - Fork 3
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
Dialogs #70
Conversation
…e alert-based confirm when deleting lists or items. fixed Loading component background color, Layout's main height, and ListButtons gap
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 |
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 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! |
Thanks for the information @borjaMarti and the update! It does seem more intuitive after the update. I used this article to dubble check the UI: |
src/components/DeleteItem.jsx
Outdated
import Confirm from './Confirm'; | ||
|
||
const DeleteItem = ({ itemName, listPath, itemId }) => { | ||
const [isConfirmOpen, setIsConfirmOpen] = useState(false); |
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.
Can you rename IsConfirmed field? Is prefix is not suggested.
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.
Done!
src/components/DeleteItem.jsx
Outdated
|
||
const DeleteItem = ({ itemName, listPath, itemId }) => { | ||
const [isConfirmOpen, setIsConfirmOpen] = useState(false); | ||
const [isSubmitted, setIsSubmitted] = useState(false); |
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.
Same as IsConfirmed
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
Type of Changes
Updates
Before
After
Testing Steps / QA Criteria
git pull
git checkout dialogs