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

Refactor frontend CSS into CSS-in-JS #741

Closed
Silvyre opened this issue Feb 19, 2020 · 7 comments · Fixed by #928
Closed

Refactor frontend CSS into CSS-in-JS #741

Silvyre opened this issue Feb 19, 2020 · 7 comments · Fixed by #928
Assignees
Labels
area: css styling Anything related to CSS styling area: front-end developer experience Helping the Developer Experience type: enhancement New feature or request

Comments

@Silvyre
Copy link
Contributor

Silvyre commented Feb 19, 2020

All of our convential frontend CSS styling solutions should be refactored into Material UI's CSS-in-JS styling solution.


(Old issue text)

Currently index.css imposes its styling rules upon all Gatsby Pages. While developing the Add Feeds component, I found some of these rules (namely the scaled-down font size) to be less-than-helpful, as I noted in my pull request:

It is worth noting that this component currently ends up being rendered quite small due to styling enforced by src/frontend/src/pages/index.css:

html {
/*assuming browser font default is 16px*/
font-size: 62.5%;
/*translates to 10px -> root font*/
}

This is not, however, the only styling issue I ran into with index.css. I noticed that, due to the stylesheet enforcing a body height of 100vh, the Add Feed component's current use of margins forces an unnecessary scrollbar to appear:

margin


There is some discussion about eventually replacing our CSS files, like index.css, with CSS-in-JS (JSS). Until we have come to an agreement to that end, I believe that refactoring our CSS files (i.e. to ensure their global styling does not adversely affect new components) prevents a good stop-gap. (Again, at this point, we definitely don't want to outright remove any CSS files...)


To summarize what I believe should be done:

  • Rework the two aforementioned styling rules within index.css to be more selective about the components it targets (namely the incoming Add Feeds component):
    1. The rule governing the global scaling down of font size should be reworked to exempt at least the Add Feeds component from being targeted. (This rule is currently causing the Add Feeds component to be rendered too small.)
    2. The rule governing the enforcement of a 100vh body height should be reworked to exempt at least the Add Feeds component from being targeted. (This rule is currently causing the Add Feeds page to have an unnecessary scroll bar.)

Please note that, however, these global styling rules should keep affecting existing/working components (as we can assume that these components have been designed with this global styling in mind)!


@yatsenko-julia expressed interest in tackling this, so I am assigning this issue to her! (Julia, please feel welcome to reach out to me if you have any questions, or would like any assistance with this!)

@yatsenko-julia
Copy link
Contributor

While I was trying to figure out why I cannot load the myfeeds page, 2 issues that you mentioned @Silvyre were resolved. What do you think if I'll try to replace CSS with CSS-in-JSS for release 0.7?

@Silvyre
Copy link
Contributor Author

Silvyre commented Mar 1, 2020

2 issues that you mentioned @Silvyre were resolved.

As far as I can tell, index.css is still imposing two overbearing styling rules on the My Feeds component (testing locally):

demo

As such, these tasks still need to be completed:

  1. The rule governing the global scaling down of font size should be reworked to exempt at least the Add Feeds component from being targeted. (This rule is currently causing the Add Feeds component to be rendered too small.)
  2. The rule governing the enforcement of a 100vh body height should be reworked to exempt at least the Add Feeds component from being targeted. (This rule is currently causing the Add Feeds page to have an unnecessary scroll bar.)

Is your experience different, @yatsenko-julia?


What do you think if I'll try to replace CSS with CSS-in-JSS for release 0.7?

I don't believe that we've come to a full agreement on this matter. While I support this initiative, others might not. Let's chat about that tomorrow, in class.

@yatsenko-julia
Copy link
Contributor

Thanks for your reply, @Silvyre. I still cannot reproduce your issue. I've tried Safari and Chrome before I saw that you are using Firefox. When I run the project in Firefox instead of styles I receive inline styles. Here is a screenshot:
Screen Shot 2020-03-01 at 4 25 19 PM

I also tried to change index.css, rebuild containers and restarted the app, but there are no visible changes. ( I added font size and color changes to the .MuiPaper-root.MuiCard-root.MuiPaper-elevation1.MuiPaper-rounded class )

Could you please help me understand why my changes do not take effect and how can I reproduce your issue?

@Silvyre
Copy link
Contributor Author

Silvyre commented Mar 1, 2020

Oh, I see; it seems like you're encountering the same phenomenon that Cindy just commented on here: #748 (comment)

You are viewing an incompletely-styled My Feeds component being served by our backend (localhost:3000), while I am being served by the Gatsby live-reloading development server on localhost:8000.

You should navigate to http://localhost:8000/myfeeds.

@Silvyre Silvyre changed the title Refactor index.css to exclude select Gatsby Pages from unnecessary styling rules Refactor frontend CSS into CSS-in-JS Mar 2, 2020
@Silvyre
Copy link
Contributor Author

Silvyre commented Mar 2, 2020

The focus of this issue has shifted. The original post has been updated accordingly.

@humphd
Copy link
Contributor

humphd commented Apr 3, 2020

Blocking #922.

@cindyorangis cindyorangis added area: css styling Anything related to CSS styling developer experience Helping the Developer Experience Priority: High type: enhancement New feature or request and removed good first issue Good for newcomers labels Apr 3, 2020
humphd added a commit to humphd/telescope that referenced this issue Apr 3, 2020
@humphd humphd self-assigned this Apr 3, 2020
@humphd
Copy link
Contributor

humphd commented Apr 3, 2020

Taking this, as we need it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: css styling Anything related to CSS styling area: front-end developer experience Helping the Developer Experience type: enhancement New feature or request
Projects
None yet
4 participants