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

Update scaffold css to use semantic classes #611

Merged
merged 5 commits into from
May 28, 2020
Merged

Update scaffold css to use semantic classes #611

merged 5 commits into from
May 28, 2020

Conversation

Terris
Copy link
Contributor

@Terris Terris commented May 27, 2020

Update scaffold css to use semantic classes - WIP!!

  • replaces utility classes with semantic classes in all scaffold generator templates
  • reduces classes used in components and migrates style declarations to scaffold.css template
  • these changes have no effect on the visual design of generated pages or components

Closes #610

@cannikin
Copy link
Member

Since everything is now prefixed with rw- do you think we still need the top level rw-scaffold wrapper around everything?

@Terris
Copy link
Contributor Author

Terris commented May 27, 2020

@cannikin

Since everything is now prefixed with rw- do you think we still need the top level rw-scaffold wrapper around everything?

Hmm, yeah. I'd certainly rather not need it. My concern is that the scaffold.css file is imported along with index.css in web's root index.js file, so they'll compete in the cascade. Without a mother class to contain scaffold only styles, it will be difficult to style html elements directly without a concern for conflicts with index.css.

In other words, .rw-scaffold a {blah:blah} feels safer then a {blah:blah}.

Thoughts?

@cannikin
Copy link
Member

Ahhh yes, I see you have styles like:

.rw-scaffold h1{font-size:1.25rem;}
.rw-scaffold h2{font-size:.875rem;}
.rw-scaffold a{color:#4299e1;text-decoration:underline}

Do you like that technique better than adding something like a .rw-link class on all <a> tags and then targeting the style on that class instead? rw-heading for <h1>, rw-subheading for <h2>... if you're going to go semantic why not go all the way? 😃 Then you could drop the rw-scaffold since everything will have a unique classname.

Right now you've kind of got a mix of "all tags of this type" and "only tags with this class" like rw-table...I feel like maybe everything should be classed? Or is that overkill?

Whenever I think too much about CSS naming paradigms like this I get frozen with indecision... there's no way to please everyone! I think that's why for my own projects I've gone all in on Tailwind, no more worrying about problems like this. 😅

@Terris
Copy link
Contributor Author

Terris commented May 27, 2020

Oh boy, yes, I battle my own indecisiveness all day and every day.

Actually, I think it's the right move to create classes for every element here so long as they are sensible (like .re-link). In the final review stages, we should probably take extra care to think about my class names—be sure they make the most sense.

Anyhow, I'll do this and see how it fits. Thanks @cannikin !

fixup! update scaffold css to use semantic classes

fixup! update scaffold css to use semantic classes

fixup! update scaffold css to use semantic classes

update scaffold components to use semantic classes

fixup! update scaffold css to use semantic classes

fixup! update scaffold components to use semantic classes

fixup! update scaffold components to use semantic classes
@Terris
Copy link
Contributor Author

Terris commented May 28, 2020

TODO

  • Confirm all class names are intuitive and semantically appropriate
  • Compare all scaffolded layouts to another project using old css
  • Browser test layouts

@peterp peterp marked this pull request as draft May 28, 2020 06:25
@Terris
Copy link
Contributor Author

Terris commented May 28, 2020

@cannikin - I feel like I've got this in a good place. Ready for your review at your convenience.

#610

@Terris Terris changed the title WIP: update scaffold css to use semantic classes Update scaffold css to use semantic classes May 28, 2020
@Terris Terris requested a review from cannikin May 28, 2020 17:47
@cannikin cannikin marked this pull request as ready for review May 28, 2020 18:43
@cannikin cannikin merged commit 1e93469 into redwoodjs:master May 28, 2020
@cannikin
Copy link
Member

Thanks so much!

@Terris Terris deleted the feat-scaffold-semantic-classes branch May 28, 2020 23:44
@thedavidprice thedavidprice added this to the v0.8.0 milestone May 29, 2020
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.

A case for a different css strategy in scaffolds
4 participants