Skip to content
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

Merged
merged 8 commits into from
Oct 3, 2022
Merged

Conversation

BrettCleary
Copy link
Collaborator

@BrettCleary BrettCleary commented Sep 29, 2022

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:

@media screen and (min-width: 2560px) {
  :root {
    font-size: 24px;
    --text-scale-ratio: 1.25;
  }
}

All fonts and spacings will update accordingly.

Other layout/UI improvements include:

  • Rounded edges on game card, search bar, and select field components
  • Changed margin/padding for some components to create a more distinguishable visual hierarchy and consistency
  • Centered elements in nav bar on game details page

Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@Etaash-mathamsetty
Copy link
Member

Etaash-mathamsetty commented Sep 29, 2022

It's looking a lot better, but I would like buttons like these to get some of the rounding treatment as well, since they currently look out of place. (issue with all themes)
image
shadows are clipping here (happens in all the themes)
image
I think the sidebar text could be a bit bigger (it's almost hard to read for me)
image

@biliesilva
Copy link

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.

@arielj
Copy link
Collaborator

arielj commented Sep 30, 2022

There's an issue with paddings and margins in the Manage Accounts screen:
image

(the user names are cut off, it should say arieljuod and ariel_juod in my case)

I think it would be interesting to have some guideline on when to use the different variables, for example, there are multiple --space-* and --space-*-fixed, I'm not sure if the idea is to simply pick the one that we see fit better from that list when we need some spacing or if there should be a reason. Like, I see --space-md is used for the border-radius of the .gameCard, but it is not used for spacing element.

@arielj
Copy link
Collaborator

arielj commented Sep 30, 2022

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.

@BrettCleary
Copy link
Collaborator Author

BrettCleary commented Sep 30, 2022

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

@BrettCleary
Copy link
Collaborator Author

is there a reason for the 3 different styles of defining these variables?

there's the ones above this comment with a fixed 1rem multiplied multiple times by (in this case) 1.2.

There's this with just fixed values (like the multiplication was already done and added here)

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, font-size sets the medium font size (1rem) and --text-scale-ratio adjusts the difference between text size steps. By keeping these as variables instead of hardcoding all the --text-<size> values in rem units, we ensure that each step is exactly 20% (if --text-scale-ratio equals 1.2) different from the next step. This ensures visual consistency and gives options uniformly along the range of text sizes we want to support. This makes it easy to iterate, add breakpoints, and prevents the situation where one font size is updated but the others are not whether that be in a breakpoint or in the _typography.scss file. It also isn't much of a limitation at all on the design side of things.

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.

@BrettCleary
Copy link
Collaborator Author

There's an issue with paddings and margins in the Manage Accounts screen: image

(the user names are cut off, it should say arieljuod and ariel_juod in my case)

I think it would be interesting to have some guideline on when to use the different variables, for example, there are multiple --space-* and --space-*-fixed, I'm not sure if the idea is to simply pick the one that we see fit better from that list when we need some spacing or if there should be a reason. Like, I see --space-md is used for the border-radius of the .gameCard, but it is not used for spacing element.

I'll fix that. Also I'll add a frontend_design_system.md file in the doc folder. Ideally we'll pick a value that matches the figma component design. In the absence of that, then yeah it'll be up to the frontend dev to "design" the element if you will. Shouldn't be too hard to test a couple values and find one that works/is consistent with other components.

As mentioned above, --space-* should be used when you want it to be a multiple of the element's font-size and --space-*-fixed when you want a px value that doesn't change.

So with border-radius, my thought was that if the element's font size were increased (let's say there's a min-width: 2560 breakpoint that increases font sizes), you would want the border radius to increase as well proportionally. Otherwise the element would look more rounded on the small screen size than the larger screen size. There might be better ways of handling this (maybe setting border radius as a %). I mostly just tried to preserve the code that was already in each element.

@BrettCleary
Copy link
Collaborator Author

It's looking a lot better, but I would like buttons like these to get some of the rounding treatment as well, since they currently look out of place. (issue with all themes) image shadows are clipping here (happens in all the themes) image I think the sidebar text could be a bit bigger (it's almost hard to read for me) image

Thanks! I'll update the .button style to include some rounding by default.

Ah I'm not on linux so I didn't catch the clipping. Good catch! All the buttons I can see on windows seem to have their shadows working. But yeah basically those elements just need some extra margin or padding. Feel free to make the changes.

Font sizes will be picked by Billie. They might look smaller if you're on a 4k screen though. We can add a breakpoint for that, but I'll wait on Billie's design.

… 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
@CommandMC
Copy link
Collaborator

The headings for Settings pages are very large now. I'd say that's fine, but some parts need a little adjustment.
The EOS Overlay section of the Advanced settings for example: Before:
image
And after:
image

These buttons in the library no longer have a backdrop, I'd say they should have one to make 'em stand out a little more:
image

@Etaash-mathamsetty
Copy link
Member

Etaash-mathamsetty commented Sep 30, 2022

It's looking a lot better, but I would like buttons like these to get some of the rounding treatment as well, since they currently look out of place. (issue with all themes) image shadows are clipping here (happens in all the themes) image I think the sidebar text could be a bit bigger (it's almost hard to read for me) image

Thanks! I'll update the .button style to include some rounding by default.

Ah I'm not on linux so I didn't catch the clipping. Good catch! All the buttons I can see on windows seem to have their shadows working. But yeah basically those elements just need some extra margin or padding. Feel free to make the changes.

Font sizes will be picked by Billie. They might look smaller if you're on a 4k screen though. We can add a breakpoint for that, but I'll wait on Billie's design.

I'm running at 1080p with a 32 inch monitor, this is probably the best case for font sizing and it's too small...
edit: seems like I didn't read your comment properly, alr I will wait for Billie's design.

@biliesilva
Copy link

biliesilva commented Sep 30, 2022

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

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.

@BrettCleary
Copy link
Collaborator Author

The headings for Settings pages are very large now. I'd say that's fine, but some parts need a little adjustment. The EOS Overlay section of the Advanced settings for example: Before: image And after: image

These buttons in the library no longer have a backdrop, I'd say they should have one to make 'em stand out a little more: image

Yes I left it as h3 like it was before. The buttons having no backdrops matches some figma work that billie has shared with me.

@BrettCleary
Copy link
Collaborator Author

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.

@BrettCleary
Copy link
Collaborator Author

I'm running at 1080p with a 32 inch monitor, this is probably the best case for font sizing and it's too small...
edit: seems like I didn't read your comment properly, alr I will wait for Billie's 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.

@flavioislima
Copy link
Member

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.

That's true. We should start using figma more and add comments there.
That would be optimal.

Copy link
Member

@flavioislima flavioislima left a 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.

@flavioislima
Copy link
Member

Actually, the only thing left is the background for the filters:
image

And here also the alignment:
image

@flavioislima flavioislima changed the title Frontend/design system [Tech/Refactor] Frontend/design system Oct 1, 2022
@arielj
Copy link
Collaborator

arielj commented Oct 1, 2022

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.

Sorry, maybe my question was not clear. I was not asking why we would have different --text-.., --space-.. and --space-..-fixed.

My question is, why do we define them in different ways like:

  --space-3xs: 0.25em;
  --space-2xs: 0.375em;
  --space-xs: 0.5em;

(all hardcoded values)

and then

  --space-unit-fixed: 1rem;
  --space-2xs-fixed: calc(0.25 * var(--space-unit-fixed));
  --space-xs-fixed: calc(0.5 * var(--space-unit-fixed));
  --space-sm-fixed: calc(0.75 * var(--space-unit-fixed));

(a hardcoded multiplier for a variable unit)

and not:

  --space-2xs-fixed: 0.25rem;
  --space-xs-fixed: 0.5rem;
  --space-sm-fixed: 0.75rem;

with hardcoded rem values (like the hardcoded em values for --space)

or:

  --space-unit: 1em;
  --space-3xs: calc(0.25 * var(--space-unit));
  --space-2xs: calc(0.375 * var(--space-unit));
  --space-xs: calc(0.5 * var(--space-unit));

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):

  --text-xs: calc(1rem / (var(--text-scale-ratio) * var(--text-scale-ratio)));
  --text-sm: calc(1rem / var(--text-scale-ratio));
  --text-md: calc(1rem);
  --text-lg: calc(1rem * var(--text-scale-ratio));

why not:

  --text-xs: Xrem; // x being whatever 1 / 1.2*1.2 is
  --text-sm: Yrem; // y being whatever 1 / 1.2 is
  --text-md: 1rem;

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 --space-unit-fixed? if so, wouldn't we need to be able to change a base unit for --space too?), or maybe it's simply a good practice when making design systems.

Copy link

@biliesilva biliesilva left a 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!

@BrettCleary
Copy link
Collaborator Author

Actually, the only thing left is the background for the filters: image

And here also the alignment: image

this is intentional and matches some figma screens @biliesilva shared with me

@BrettCleary
Copy link
Collaborator Author

Sorry, maybe my question was not clear. I was not asking why we would have different --text-.., --space-.. and --space-..-fixed.

My question is, why do we define them in different ways like:

  --space-3xs: 0.25em;
  --space-2xs: 0.375em;
  --space-xs: 0.5em;

(all hardcoded values)

and then

  --space-unit-fixed: 1rem;
  --space-2xs-fixed: calc(0.25 * var(--space-unit-fixed));
  --space-xs-fixed: calc(0.5 * var(--space-unit-fixed));
  --space-sm-fixed: calc(0.75 * var(--space-unit-fixed));

(a hardcoded multiplier for a variable unit)

and not:

  --space-2xs-fixed: 0.25rem;
  --space-xs-fixed: 0.5rem;
  --space-sm-fixed: 0.75rem;

with hardcoded rem values (like the hardcoded em values for --space)

or:

  --space-unit: 1em;
  --space-3xs: calc(0.25 * var(--space-unit));
  --space-2xs: calc(0.375 * var(--space-unit));
  --space-xs: calc(0.5 * var(--space-unit));

with using a multiplier in the base unit (like for --space-.-fixed)

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.

And this other group seems to do the opposite (hardcode the unit and parameterize the multiplier):

  --text-xs: calc(1rem / (var(--text-scale-ratio) * var(--text-scale-ratio)));
  --text-sm: calc(1rem / var(--text-scale-ratio));
  --text-md: calc(1rem);
  --text-lg: calc(1rem * var(--text-scale-ratio));

why not:

  --text-xs: Xrem; // x being whatever 1 / 1.2*1.2 is
  --text-sm: Yrem; // y being whatever 1 / 1.2 is
  --text-md: 1rem;

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.

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 --space-unit-fixed? if so, wouldn't we need to be able to change a base unit for --space too?), or maybe it's simply a good practice when making design systems.

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 BrettCleary merged commit b6f3da7 into beta Oct 3, 2022
@BrettCleary BrettCleary deleted the frontend/design-system branch October 3, 2022 18:20
@arielj
Copy link
Collaborator

arielj commented Oct 3, 2022

@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 oh, we can simplify this and spend time refactoring something that shouldn't change. I don't know if this is documented somewhere else (maybe in figma?) or if there should be comments in the CSS file to make it clear why we defined them like this and what are use cases when those variables would be overridden (like... do we want to change the 1.2multiplier anywhere? if so, when/where? it should help when reviewing things to if somebody tries to override that)

@BrettCleary
Copy link
Collaborator Author

@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 oh, we can simplify this and spend time refactoring something that shouldn't change. I don't know if this is documented somewhere else (maybe in figma?) or if there should be comments in the CSS file to make it clear why we defined them like this and what are use cases when those variables would be overridden (like... do we want to change the 1.2multiplier anywhere? if so, when/where? it should help when reviewing things to if somebody tries to override that)

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants