Closed jssckbl closed 3 years ago
Visit the preview URL for this PR (updated for commit da30683):
https://tcl-23-shopping-list--pr28-jc-jw-list-items-by-4q92q112.web.app
(expires Sat, 29 May 2021 14:57:54 GMT)
š„ via Firebase Hosting GitHub Action š
I think we are ready for approval!
To test for accessibility, we used WebAIM WAVE as a Chrome extension, as well as the VoiceOver option available on Mac.
NB: There is a lot of use of "I" in the PR comments above. As a group effort and also since it's unclear who's communicating, 'we' may be more appropriate.
I've underscored in pink the new UTC DateTime format
The image shows a timestamp with a UTC offset but is not UTC itself. And yes it's definitely best to use ISO-8601 timestamps! š
There is a lot of logic within the presentational JSX of src/pages/List.js
that is making it difficult to understand the changes being made. Typically logic is extracted from the JSX render/return to functions and variables within the component to keep the visual presentation code clean. Depending on team time, you might be doing your reviewers and your future self a big benefit by taking some time to not change the logic, but refactor it for legibility.
cc @jssckbl @jamesncox
@skylerwshaw regarding the issue about logic in JSX I 100% agree with you, but the problem was the the logic being processed outside the return statement before the connection to Firebase could be fully established, thus throwing an error (I believe an undefined error). I suppose I could create four different functions that I call in the return statement, one for each filter. Which would be a bit more readable. Can we escalate that to a different, yet-to-be-created issue?
@skylerwshaw requesting another review! I have extracted all of the logic out of the return and even DRYed up the render of the four unordered lists. Thanks!
Description
We added functionality in
List.js
so it can filter list items alphabetically. It can also move an item into one of 4 different lists based on how often it is purchased. The 4 groupings are:List.js
file is getting quite lengthy. UPDATE May 16, 2021 - some refactoring has occurred!To test for accessibility, we used WebAIM WAVE as a Chrome extension, as well as the VoiceOver option available on Mac.
Related Issue
closes #13 closes #29
Acceptance Criteria
[x] Items in the list are shown as visually distinct (e.g., with a different background color on the list item) according to how soon the item is expected to be bought again: Soon, Kind of soon, Not soon, Inactive
[x] Items should be sorted by the estimated number of days until next purchase
[x] Items with the same number of estimated days until next purchase should be sorted alphabetically
[x] Items in the different states should be described distinctly when read by a screen reader WORKING!
Type of Changes
Notes
aria-label
. But I think I realized that we shouldn't usearia-label={
grocery-item${++index}}
andid={
grocery-item${++index}}
because the screen reader is reading the index, not the same. So maybe something likearia-label={item.data().item_name}
andid={item.data().item_name}
makes more sense? We will have to test it to be sure of course!UPDATE as of May 16, 2021:
aria-label
removed. It was unnecessary for this project at this time in assisting the screen reader.alphabetizeListItems()
that we can re-use for rendering the various list items by their suggested purchase time. Something likefilterItemsByLastEstimate()
. But I need help and guidance with this one.Before
Items were listed in a seemingly random order. When the user entered an item into the list, there was no visual showing which grouping it had been put into.
After
Items are now grouped based on frequency of purchase. Within each grouping, items are listed alphabetically.
Testing Steps / QA Criteria
On
main
rungit pull
Run
git checkout jr-jw-list-items-by-frequency
Run
npm install
Run
npm start
Once you are at
https://localhost:3000
, clear any existing tokens from yourlocal storage
. The screenshot below can assist you in clearing any tokens when using Chrome as your browser.Once you are on the main page, it is advised to create your own new token, and to add a few items to each of the 3 categories -
soon (0-7)
,kind of soon (8-30)
,not that soon (30+)
. Don't forget to include capitalization and punctuation as well with the grocery list items you add.Once you have created a grocery list with various items in the green (0-7), purple (8-30) and red (30+) day categories, you will see that items in each grouping are alphabetized. Items with a checkmark that are gray are considered
inactive
and have been purchased in the last 24 hours, or have exceeded their assumed time frame of when they would be purchased again.We used the native VoiceOver function on a 2018 MacBook Air, running macOS Big Sur, Version 11.3.1 to test the screen reader. The following are directions to use that screen reader:
command
+spacebar
accessibility
,enter
VoiceOver
is highlighted (as seen below)Enable VoiceOver
tab
,enter
, and the arrow keys to navigate the fields on the screen.X
in the upper left corner of the gray VoiceOver box.FINAL UPDATES
I have addressed ALL the AC and everything is working as intended. The only thing I have NOT completed is refactoring the WET code to be more DRY with a switch statement or anything like that.
Check out the screen shots to see that everything is rendering on the screen correctly, still alphabetized, even if some of the items are capitalized, and that the items become inactive under the AC's stated conditions.
Okay Friends, I have updated A LOT of the codebase here. The main thing that I did was to break down and use a package for DateTime called Luxon https://moment.github.io/luxon/docs/manual/tour.html
If you look at the Firebase screen shot above, I've underscored in pink the new UTC DateTime format. Apparently, this is the industry standard way to store times in databases. Luxon gives us a ton of ways to work with their DateTime object, and this one is achieved by
.toString()
.I am still storing the
last_estimate
as an integer representing days, which is total milliseconds divided byconst millisecondsInADay = 86400000;
So now we have a more readable timestamp in the database, and still able to compare last_estimate by days.
compareTimeStamp()
, which determines whether or not a checkmark stays checked or not is completely revamped:An item in the database which is less than 24 hours old will remain checked, but now it's MUCH easier to follow the logic in the function and the properties in Firestore
Another feature I successfully implemented is the accessibility/screen-reader working correctly. Instead of using
aria-label
, I opted for<label>
. I still have FOUR different filters and maps, but here is an example of one of them:I updated the id to use
id={doc.id}
mirrored by thehtmlFor={doc.id}
. And now the screen reader knows which check box to associate with which{doc.data().item_name}
.Conclusion
I think we are good to go to merge. Please let me know if you see anything I can fix/update! If you test this branch with the live Github link, make sure to start with a new token so the data in Firestore is not stale.
I have added a ton of comments that I hope clarify the code, but if there isn't anything you don't understand, let me know!