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

Alerts in-app #74

Merged
merged 5 commits into from
Apr 5, 2024
Merged

Alerts in-app #74

merged 5 commits into from
Apr 5, 2024

Conversation

borjaMarti
Copy link
Collaborator

Description

This PR displays alert messages from adding items or sharing a list in-app, instead of with the browser's native alert() function.

Related Issue

closes #73

Acceptance Criteria

  • When sharing the list with another user, all messages are displayed below the component, instead of in an alert message.
  • When adding an item, the confirmation message is desplayed below the component, instead of in an alert message.

Type of Changes

Type
✨ New feature
🎨 Enhancement

Updates

Before

image

image

After

image

image

Testing Steps / QA Criteria

  • git pull
  • git checkout alert-messages
  • Add items and share a list, observe resulting message.

@borjaMarti borjaMarti self-assigned this Apr 3, 2024
Copy link

github-actions bot commented Apr 3, 2024

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

https://tcl-71-smart-shopping-list--pr74-alert-messages-hjj39zxe.web.app

(expires Thu, 11 Apr 2024 09:35:44 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1e7ade9d0f374c4ddb5d7ab6fc541062fc7a1ab4

Copy link
Collaborator

@vivitt vivitt left a comment

Choose a reason for hiding this comment

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

Thanks Borja, the changes look nice!

The only thing I would suggest is to add a fixed height to the section elements (I tested with h-60 and seemed to work fine) in ManageList.jsx, to avoid the layout shifting when showing/not showing messages.

Copy link
Collaborator

@BikeMouse BikeMouse left a comment

Choose a reason for hiding this comment

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

Thank you! This is so much better!

@borjaMarti
Copy link
Collaborator Author

borjaMarti commented Apr 3, 2024

Thanks Borja, the changes look nice!

The only thing I would suggest is to add a fixed height to the section elements (I tested with h-60 and seemed to work fine) in ManageList.jsx, to avoid the layout shifting when showing/not showing messages.

Hey @vivitt! I played with it a bit and arrived at something that prevents layout shift (most of the time, if the item name is too long it will cause the element to take up multiple lines, so that's an edge case) while not creating too much blank space. Tell me what you think if you check it out!

src/views/ManageList.jsx Outdated Show resolved Hide resolved
@borjaMarti borjaMarti requested a review from elitcenk April 4, 2024 10:15
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.

Thank you for adding this update!

Copy link
Collaborator

@elitcenk elitcenk left a comment

Choose a reason for hiding this comment

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

Thanks

@borjaMarti borjaMarti merged commit 1f8d279 into main Apr 5, 2024
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.

32. Integrated messages instead of alert()
5 participants