Skip to content

Conversation

@alexlafroscia
Copy link
Contributor

Generally, <a> should be avoided with href="#" since it's not "semantically correct" to use a link when no navigation occurs. A <button> is better semantically. It looks the same, just without the navigation.

@@ -0,0 +1,5 @@
.docs-demo__snippets-nav-item {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an outline-none class defined here that we can use instead of BEM

I know discovery is a problem, I have some ideas for how to fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. I'll clean that up.

Yeah, diving into the CSS has been a little hard. I have no idea which (of the non-BEM classes) are part of this app's styles and which are part of Tailwind. There are a ton of BEM classes in the templates that aren't actually defined in the CSS anywhere, and a bunch of classNames in JS that are just commented out 🤷‍♂️

@alexlafroscia alexlafroscia force-pushed the avoid-a-tag-without-navigation branch from 5bf6fe4 to 94cfa47 Compare March 27, 2018 01:00
}
.outline-none {
.outline-none,
.outline-none:focus {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to also override the default for the :focus state, otherwise it would be outlines while clicked on and then lose focus. Probably not the visual that is desired.

@samselikoff samselikoff merged commit c212bf7 into ember-learn:master Mar 29, 2018
@samselikoff
Copy link
Contributor

@alexlafroscia thanks!

Are there lots of BEM classes left? I wanted to ship the redesign so I didn't get to get rid of all of them, but I thought I got most of them. I think there's a bunch related to the auto-generated API docs that I haven't cleaned up yet.

I think it's a tradeoff and I saw those BEM files getting bigger and bigger, and us not sharing styles, so I'm still confident a limited set of functional styles will be easier for folks, but I'm eager to hear all feedback. I know it's not there yet but I think we can get there.

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Mar 30, 2018

I dunno about "a lot" but I definitely saw some classes commented out when I was looking around.

A lot of my perspective just comes down to preference, but I'll give it anyway. I hate global CSS classes. Discoverability of utility classes is really low unless you're intimately familiar with the tools you're using. The cascade is hard to fight against. I feel like it inevitably leads to .lots { .of { .nested { .classes { } } }.

I vastly prefer keeping 100% of the styles locally-scoped (through something like ember-css-modules, ember-component-css or my own ember-emotion) and composing global styles into those local classes if need be. There should (IMO) pretty much only ever be one local class applied to each element (except for maybe modifiers for states), although that local class might be composed of multiple other classes. Those should then be imported (both ember-css-modules and ember-emotion can support this) from the place where they're defined, so it's obvious where the definition is coming from. Global CSS classes only as a last resort.

All this to say, stuff like Tailwind rub me the wrong way in the way that they just give you a bunch of global CSS classes. Coming into a project like this, I have no idea what is a locally-defined utility or what is coming from Tailwind unless I first grep for it in the codebase, fail to find a definition, then go find the Tailwind docs to figure out what the class does.

I'm almost certainly taking this too seriously though, so don't worry about it too much.

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.

2 participants