-
Notifications
You must be signed in to change notification settings - Fork 2
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
16. Layout Changes and Tailwind Implementation #40
Conversation
Visit the preview URL for this PR (updated for commit a1c223c): https://tcl-66-smart-shopping-list--pr40-an-layout-pijdok8q.web.app (expires Mon, 01 Apr 2024 18:48:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2dc16cb2a79cbd6723fdc511b73e0743df1d18d0 |
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.
Aloooooe!! Great work! Thanks for all your efforts!! I left a few comments in the code at places where I though we could add some specificity to user messages. These might just be my preference, so not a hard stop.
@@ -27,26 +31,39 @@ export function List({ data, listPath, loading }) { | |||
|
|||
return ( | |||
<> | |||
<p> | |||
Hello from the <code>{listName}</code> page! |
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 think this should say 'list' instead of 'page'. Maybe even 'your' instead of 'the'?
welcome to the 'badger' list!
welcome to your 'badger' list!
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.
This makes sense! I like it
src/components/ListItem.jsx
Outdated
// Join the words back into a single string | ||
return capitalizedWords.join(' '); | ||
}; | ||
|
||
const handleDelete = async () => { | ||
try { | ||
if (window.confirm('Are you sure you want to delete this item?')) { |
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.
Maybe we can add the name of the item we are deleting here.
are you sure you want to delete {name} ?
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 also like this!
@@ -0,0 +1,28 @@ | |||
/** @type {import('tailwindcss').Config} */ |
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 common colors be added here as well? so that way if we want to change the main font color or the main background color we can go to one file? Also not sure if you like the font family or not, but I think that the font in the figma design is better than the one we have now.
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.
Yep! I can add some variables and change things.
Description
tailwind.config
file with some updated variables for screen sizing and fontsmanage-list
view from nav bar, and movedaddItem
logic from that view to the list viewmanage-list
file because it had the invite feature, and we might want to migrate that somewhere else, but currently there is no share button displayed anywherehome
viewhelpers
to have hex codes for colours to be more accessible in contrastcapitalizeFirstLetterOfEachWord
function to capitalize first letter of each word for list items when rendering listNote: I did not style buttons and only styled inputs in order to be more responsive.
It's a lot of code changes, sorryyyyyyy <3
Related Issue
closes #38
Acceptance criteria
Type of Changes
accessibility, enhancement, and refactoring
Updates
Before
After
Updated UI of list with no items:
Testing Steps / QA Criteria