-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
[Tech/Refactor] Frontend/design system #1851
Conversation
…ign system, other UI layout improvements
We are planning to change the visual of the sidebar to improve readability and hierarchy in the next big release. Also, we will check again the sizes for it. I would like to check also if we will return to using rounded edges, if yes we need to match other components/screens to maintain consistency. Right now the cards and buttons have no roundness, for example, also we had roundness before and we decided to change it in the last visual change that we made. |
Personally, I vote for no rounded corners, but it's just my preference (I don't like them), I don't have any argument against or in favor. Maybe more subtle rounded corners? not completely rounded pill-style sides. |
Rounded is more modern imo. Scrolling through dribbble, pretty much all the designs are rounded to varying degrees. I'd be fine with reducing it on some components though, especially for some of the components on the settings screens. I also didn't know there was already a discussion with regards to this. Apologies. I wasn't trying to make any unilateral decisions |
I'll address this here so it doesn't get buried Yes. This is all very intentional. Let me first explain in detail how it's supposed to work. Not to be pedantic, but the type system is adjustable with two variables. Like you've seen, Now, the spacing system is first specified in em units. This means that it is a multiple of that element's font size. So let's say you have h6 at 1 rem (16px) and want margin-bottom to be 50% of this font size. By setting margin-bottom = 0.5 em (--space-xs) on the element, you accomplish this. When someone introduces a breakpoint later and changes the font, the margin-bottom won't have to be updated because it's specified in em. (So you probably won't have to update these css vars in media query breakpoints.) By creating css variables for these sizes, we constrain the components to use one of these predefined sizes, which again enforces visual consistency and makes the code much more maintainable. Designers can now iterate on the entire app's margins and paddings by tweaking these values. The fixed spacing system is intended for margins and paddings that shouldn't change based on the font size of the element. So if you want something to truly be 4px instead of 25% of a 1rem, use this. I did my best to respect the margin/padding choices of fixed vs not fixed for each element when refactoring. One caveat is that I actually just realized I defined the fixed system wrong. Earlier I had the font-size variable on the body element instead of the root element. There's really no reason to have it on the body instead of the root (since we zoom instead of changing the root font size) and it's simpler to set it on the root. I'll change 1rem on the fixed system to 16px and that will fix that issue. |
… and select field components, added some default button rounding, fixed issue with usernames on accounts page, changed fixed spacing system to use px instead of rem
No need to apologize :) About the roundness, we can return to use it but would be nice if we change some elements like game cards, etc, and use a max of 3px and reduce it in small elements to maintain the subtlety. |
I want to clarify to everyone that this is not intended to be a dramatic overhaul of the current UI. It's the first step towards a design system that is maintainable. Also (fully guilty here as well) quote replying your personal opinion on each individual element's appearance is really a horrible way to design this app. From what I've gathered, BIllie is the lead designer and we should defer to her. If you want to participate in the UI discussion, a productive use of our time would be adding you to the figma file and having you work with Billie to update the design. |
It isn't too small though. It matches the figma file almost exactly, which specifies 16 px and 14 px for these sidebar headings and subheadings. |
That's true. We should start using figma more and add comments there. |
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.
Looks good.
Tested here and could not find any more issues.
I think in the long run having something like this is more sustainable and scalable and thanks for documenting it as well.
Sorry, maybe my question was not clear. I was not asking why we would have different My question is, why do we define them in different ways like:
(all hardcoded values) and then
(a hardcoded multiplier for a variable unit) and not:
with hardcoded rem values (like the hardcoded em values for --space) or:
with using a multiplier in the base unit (like for --space-.-fixed) And this other group seems to do the opposite (hardcode the unit and parameterize the multiplier):
why not:
Sorry if this is answered in your comment, it's still not clear to me if there is a use case in heroic to have that much flexibility (would we be changing |
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 tested it here on a Mac and is fine!
this is intentional and matches some figma screens @biliesilva shared with me |
Oh okay. Sorry I must have misunderstood the first time around. I just hardcoded the relative space values to try and best match the figma files/what was already coded. We can do the multiplier approach if you want. It doesn't matter to me. Multiplier approach would make it easier to tweak for the designers, but I think they (i.e. relative and fixed spacing values) won't need to be changed very much.
The text sizes are different because there are two variables that can change them, root font size and the multiplier. This is important as we can change the spacing and the base size independently and with only two values. Imposes a good constraint (fixed ratio between each successive font size) for visual consistency without overly restricting our designers. Also makes it super simple to set breakpoints because you only have to set two values and everything updates automatically.
If we hardcoded the text sizes, it would actually be more flexible than the current system and open us up to the possibility of improperly defined breakpoints/inconsistent sizes. We probably shouldn't change the fixed space units much in the breakpoints as that goes against them being fixed. We don't need to change the relative space units in the breakpoints because they change automatically with their element's font size since they're em units. Hope I answered your questions! I'm available on the discord too. |
@BrettCleary oh ok, got it, I don't have a preference, I was just wondering why were those declared differently, I'm worried that somebody could go there and think |
Yeah. The text multiplier would be changed in the breakpoints. This is documented in the src/frontend/index.scss file. I can also mention it in the doc/frontend_design_system.md file. I can add some comments to the css files with regards to the systems. These values are more so up to the designer (or frontend dev that is working with the designers). |
Improved the frontend design system and introduced SASS.
src/frontend/styles
now contains sass files that define the typography, spacing, button, and color systems. Other app-wide styles can be defined here in the future using the full power of SASS.This parameterizes the design system and clearly defines it in one place while preserving existing features like allowing the user to change the primary/secondary fonts.
Before this, font-sizes, weights, margins, and paddings were hard coded in css files for each component. This makes it very difficult to iterate upon, add screen size media query breakpoints, and disrupts the vertical rhythym and visual consistency as the spacing between elements and font sizes are not standardized.
Also we can now support breakpoints by only changing two css variables. Add the following to
src/frontend/index.scss
to create a breakpoint:All fonts and spacings will update accordingly.
Other layout/UI improvements include:
Use the following Checklist if you have changed something on the Backend or Frontend: