-
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
[ISSUE 19] Pin and place important lists on top #70
Conversation
- display pre-sorted by importance lists; - pass isImportant prop to each SingleList component
Visit the preview URL for this PR (updated for commit cd8801c): https://tcl-75-smart-shopping-list--pr70-nk-important-lists-o-0aqen666.web.app (expires Tue, 08 Oct 2024 21:15:03 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1f1fd53c369e1fa31e15c310aa075b4e8f4f8dde |
setImportantList(path); | ||
}; | ||
|
||
const pinStyle = isImportant ? 'visible' : !isHovered ? 'hidden' : 'visible'; | ||
|
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.
pin is displayed based on whether it is:
- important,
- not hovered,
- hovered
src/components/SingleList.jsx
Outdated
fontSize="large" | ||
className={pinStyle} | ||
onClick={handleImportantList} | ||
/> |
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.
the icon is handling setting selected list as important
const [importantLists, setImportantLists] = useState( | ||
() => JSON.parse(localStorage.getItem('importantLists')) || [], | ||
); | ||
|
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.
use state to display real-time changes to local storage where we store an array of objects:
{
path: string,
datePinned: Date
}
Where :
- path is the path to a list that was marked as important,
- datePinned stores a date that the list was marked as important. datePinned is needed to be able to display the lists in the reverse add order
return 1; | ||
} | ||
}; | ||
|
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 tried to use a ternary and it wasn't working at all!
I would consider the user experience here. Unless you have a perviously pinned list, or you hover over the lists a user would not know that you can pin a list. One of the ways that we can visually indicate this ability is to have an "empty" or "outline only" version of the pin icon on every list item, and have the ones that are pinned filled in. The other option is to have the pin look more like a "button", and indicate pinned status through highlighting. I've added a couple different examples that might help you visualize what I'm talking about:
Also, it's important to remember that visual representation is not the only thing that's important for our app. For any icons, we should also make sure to try to add alt tags and aria-labels to make our visual indicators accessible. I linked a good article that highlights the differences between the 2 and how to use them together to make visual items on a page more accessible. |
I agree, I will update this to be buttons for both delete and pin icons, and I will add aria-labels and alt tags! Thank you! |
…I Tooltip and aria-labels. Outlined pin buttons are now visible at all times, and on click they become filled buttons
src/components/SingleList.jsx
Outdated
marginBlockEnd: '0', | ||
}; | ||
|
||
const tooltipTitle = isImportant ? 'Pinned' : 'Not pinned'; |
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.
Instead of thinking of the tooltip as labeling this icon in this case, this icon actually is a button that has a function. Unlike the due soon, etc. labels, I think the tooltip should indicate to the user what would happen if it gets clicked. This is because they can already see if the item is pinned or unpinned visually. The concept of this icon button tooltip is similar to how you labels your aria label for each state.
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.
Combined tooltipTitle and iconButtonAreaLabel into importantStatusLabel!
const importantStatusLabel = isImportant ? 'Unpin list' : 'Pin list';
…usLabel; simplify MUI icons imports
Description
This code proposes a custom hook that manages and sorts lists that were marked as important
useImportance
hook to manage a list of items and their importance status.I am using localStorage to locally store references to important lists - like this we don't have to make any additional calls to firebase.
I wanted to try and sort items the way that we originally wanted to implement urgency of items. I read a bit more about how sorting works in JavaScript and I think this time everything works correctly, and I am happy about this.
Would love to change UI based on feedback
Still working on tests
Related Issue
closes #65
Acceptance Criteria
Type of Changes
enhancement
Updates
Before
After
Testing Steps / QA Criteria