Style the 'All Raids' page #67
Conversation
There was a problem hiding this comment.
Off the top of my head, things that stand out:
- Filters and Raids labels aren't baseline-aligned with each other - it looks quite strange with filters hanging off in space on its own
- The gap between the filters and the raid cards is off-puttingly large IMO
- The filter only applies on pressing the enter key... This is very strange behaviour for me. I was expecting live filtering or something (not sure if that's due to my original thining about how it should work or not)
- Now that I actually see the tabs in real life, I realise the visual affordance of just bold text vs not bold text is horrible 🤦♂️ That's on me, though
Some other requests:
- We already have a Card component - can you use that instead?
- Please use a
$prefix for styled elements - Please place the styled elements after the component
|
Thanks for working on this! Teaching me about how bad my wireframes are 😛 |
There was a problem hiding this comment.
Other than what @nobrayner has already mentioned, the things that really stick out to me are:
- there needs to be a max width on the page content, otherwise the 'Raids' and 'Filter' sections just spread apart continuously as the screen widens
- responsiveness breaks between 800px and 640px screen width
- I think we should shoot for everything being responsive down to 320px screen width
|
@codyarose @nobrayner I also noticed pixels are being used, should we not use REM? 🤔 Asking since no one of you mentioned that, hence am wondering. |
Spacing, font size, or any other properties that could use our sizing variables should be using those variables. If it's a property like width that doesn't have a variable, we should prefer rem for consistency |
| <div> | ||
| <label | ||
| htmlFor="dungeonInput" | ||
| className="block text-sm font-medium text-gray-700" |
There was a problem hiding this comment.
i don't see styles for these classes and we should be using styled components, no?
| </Tabs> | ||
|
|
||
| <Raids> | ||
| {filteredData.map((s) => ( |
There was a problem hiding this comment.
im not sure if everyone agrees, but im not really a fan of single letter variables.
| <span role="img" aria-label="cross"> | ||
| ❌ | ||
| </span> | ||
| <span> No raids here </span> | ||
| <span role="img" aria-label="cross"> | ||
| ❌ | ||
| </span> |
There was a problem hiding this comment.
We have the Emoji component for this :)
|
Thanks everyone for the feedback. Unfortunately, I don't know when I will find some more free time to address these changes. So, if this is a time sensitive issue or someone is ready to work on this, please do it. Until then, I am closing this PR, so that I am not holding off anyone. Will reopen it if/when I fix all the things that are needed to be fixed. |
Fixes #17