Finished initial pass on Dashboard My Stories Page UI Grid View#926
Finished initial pass on Dashboard My Stories Page UI Grid View#926BrittanyIRL merged 2 commits intomasterfrom
Conversation
|
Size Change: +902 B (0%) Total Size: 511 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Did you experience issues with this otherwise? I thought our SVGO implementation automatically changes these attributes to use currentColor.
There was a problem hiding this comment.
Oh I didn't even realize it did that. Yes, we have had to change it to currentColor otherwise it uses the color listed.
BrittanyIRL
left a comment
There was a problem hiding this comment.
Looks great! I have a few minor things that might be helpful. Looks like there's a recent conflict in package.json. Thank you for the updated names on the grid!
There was a problem hiding this comment.
I think making the svg width and height a constant that you can throw in for the multiple use cases here would make updates more straight forward.
There was a problem hiding this comment.
I like the idea of an icons metrics constants obj.
There was a problem hiding this comment.
could you add an aria label to this button to give it context for what it is toggling?
There was a problem hiding this comment.
That's a wonderful idea.
assets/src/dashboard/constants.js
Outdated
There was a problem hiding this comment.
should this be uppercase?
mariano-formidable
left a comment
There was a problem hiding this comment.
Only one non-blocking naming nit you can take or leave! Excellent work 🎉
There was a problem hiding this comment.
Naming nit you can choose to ignore, but for a second I found myself reading state as a react state variable 😱 Just for clarity sake, do you think it might read better to call it mode and then LIST_STATE would also be LIST_MODE (and all references to it updated appropriately)? That way the verbiage (even when talking about it) won't every be confused with react state?
2734dce to
5d33c45
Compare
BrittanyIRL
left a comment
There was a problem hiding this comment.
Thanks for the quick updates!
Green to show hit target for toggle button since the icon is quite small.