Make Chippy a component #38
Conversation
drakenguyen4000
requested review from
yiremorlans,
alucernoni,
ashleyvalentine and
DwightTheShark
|
Visit the preview URL for this PR (updated for commit fd6847b): https://tcl-56-smart-shopping-li-ffe7d--pr38-dn-make-chippy-a-bsb55zea.web.app (expires Tue, 30 May 2023 04:35:10 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c51cec5927ae3f1253a2134be95c1a07393f9a7 |
| helvetica neue, helvetica, Ubuntu, roboto, noto, arial, sans-serif; | ||
| /* font-size: 1.6rem; | ||
| line-height: 1.4; */ | ||
| font-family: 'Edu VIC WA NT Beginner', cursive, -apple-system, |
There was a problem hiding this comment.
After our accessibility workshop, I have been viewing web development with a different lens and I don't believe this font choice is accessible. On top of that, if the user's browser does not support that font and it defaults to cursive (the following backup font) it will become 100% inaccessible. Choosing the right font
As a personal preference, I really like the handwriting style font and it goes with the theme of our app, but " Cursive or handwriting style fonts also make the text inaccessible, and hence they are not the best fonts to use for a website." I would like it if our app had two different fonts instead of one font for all the text. For example, the title and navigation will have one font that is bold. The rest of the headers will have the same font but in regular, and then our shopping list items will be an entirely different font in regular.
Fontjoy This is a great website for font pairings and it takes you straight to the google font. Let me know what you think!
There was a problem hiding this comment.
I agree with Yire's assessment on the typography.
There was a problem hiding this comment.
Noted. I'll look for something that more accessible by all users and implement the other changes recommended. This is for chippy's separate component. I'll make the font styling changes in the font styling pull request instead.
| : `Yummy! That list is looking good! Did you know that you can use the filter to search within ${listName}?` | ||
| } | ||
| /> | ||
| <div id="listMods" className="m-2 pt-2"> |
There was a problem hiding this comment.
I think the placement of this is much better suited to where you moved it!
There was a problem hiding this comment.
The changes you've made to the application are a big improvement to how it looked before! Thank you for giving it a more polished look--I think it looks infinitely better already and I feel much more confident about presenting something that looks like this. I also think isolating Chippy into its own component was a great idea.
I do want to make a slight request for you to change the font to a more accessible choice. I did make a comment about having a font pairing, but if you want to keep the single font, I respect your stylistic choice. :)
Description
Turns Chippy into a component and have him handle app messages.
Related Issue
#37
Acceptance Criteria
Type of Changes
Updates
Before
After