Add unique classes to components #1361
Conversation
|
@djvs Disclaimer: I'm very new to this project, so I can't speak for the code maintainers (@dessalines and @SleeplessOne1917), but here's my read on the situation: Because the project is moving so fast and because there will be so many different people working on it, I think sticking with Bootstrap classes is an intentional decision to (a) reduce overhead for pitching in (no learning curve on class naming conventions or an extra layer in the stack) and (b) prevent inconsistencies in approach or naming. (I have a general distaste for Bootstrap, too, but I think its use may be warranted in this case.) Regarding the issue of elements not being easily targetable for custom styling, I think the general approach right now is, "If it can't be done with Bootstrap, don't do it (for now)." Much of Bootstrap can be customized with theming (there are other efforts going on there AFAIK). See the conversation in #863 It might be a good idea to update the Contributing guide to discuss this. |
|
I think there's a worthwhile middle ground to explore here. I understand the point that @dessalines is making in that he doesn't want to bind the base themes to custom classes because the core of lemmy-ui should be able to painlessly adopt any Bootstrap theme. However I also agree with everyone else in the community who says that doing custom theming, which is a valid and clearly highly desirable goal, is made unnecessarily difficult by the current lack of semantics in the markup. I don't see why it would not be a viable path forward to insist that the base Lemmy themes be only based on Bootstrap standard classes, and that any markup added or changed adhere to the Bootstrap structure and styling, while also providing hooks for users who want to go beyond Bootstrap. This includes far more than just theme developers, but also people making userscripts and browser extensions. If the goal here is to support user choice first (be it end-user or instance host), while also keeping the codebase clean and accessible, I think there's a fairly easy to reach line that accomplishes both of those goals. We can have our stock experience that can seamlessly slot in a Bootstrap theme for those who just want to ship the defaults with maybe some light color swaps, while also enabling an ecosystem of users adding increased value and customization to either their individual or hosted experience. |
|
Agreed, I have no idea where the naming conventions for these classes lives/came from but it needs to be much better documented if it's going to be merged in imho. The class names in this PR are confusing, sometimes abbreviated sometimes not, undocumented suffixes, does not seem to adhere to BEM or any other naming convention I'm familiar with. |
You wrote this seconds after I posted my response. I also completely agree with this post. If we are going to extend courtesy to customization it should be through sane and sensible naming conventions that align well with the existing Bootstrap standards. |
|
Well, in general I would advise against using Bootstrap in the first place. That creates a baked-in opinionated approach to layout which limits themes' ability to handle responsive design. That aside, the current reliance on Bootstrap classes, with virtually no other classes anywhere else in the codebase, as Klobenda mentions in the other thread, is completely hostile to any third party themes. People inevitably are forced to create 6-element-deep selectors to target specific elements which will break suddenly and non-debuggably with any change to the components. I have literally already had to do this a few dozens of times for my user-styles CSS (linked via the issue linked above). The rule of thumb after my PR here is to try to limit necessary selectors to 2-3 levels deep if possible. There is also a serious issue with lack of contextualization - I noticed I had a hard requirement to actually reference "comments on a post page", "comments on a user page", etc. to create reusable styles, but this was impossible, and had to be constructed using a basically random sequence like "#app > div > ul > li > div.mr-3 > li". The naming convention I used is basically just "create unique names and try to namespace them". BEM tends to be extremely verbose and unpleasant to work with, and unnecessarily confines elements to contexts that may be more harmful than useful. All I did was basically namespace particular elements with something close to the name of the wrapping component, detail rows, buttons, li's etc per name to make them individually addressable in importance places, and the outermost component (or something very likely to be branched off) is prefaced with "cnt-". "-w" is a similar suffix for wrapping components inside one of the containing/component-level ones. I'm happy to change those, I imagine the "cnt-" one is least palatable. Keep in mind that the length of class names increases payload size in general, so there's a balance here between specificity/verbosity and conciseness. But, in general, the classes/IDs just refer to "specific things on whatever it is your web app does", so you can generally get away with "just use alphanumeric + hyphens, and maybe underscores, and don't create names that are so vague you accidentally reuse them". I wouldn't really overthink it past there. edit: To walk that back slightly, BEM is fairly workable if the block scope isn't overly broad, which is the approach I went with for the later implementation here (mostly the same as component-level scope). |
I also agree with this, but it's not my place to drive that large of a change.
This is something I tried to help with with the changes I made in #1327, but I went for a much sparser approach trying to only focus on major areas rather than tagging as thoroughly. I do hope we can reach a consensus on this with the maintainers.
I did read this in my head as c*nt 😅 |
|
Well, I mean more that cnt- is a prefix, although it is done that way to make it explicit it's denoting a section. It's short for "container". "w" for wrap, "err" for error, etc.. |
The project is already heavily using Bootstrap, so dropping it will require quite a bit of planning. I agree that relying on Bootstrap can be problematic but it's what we've got.
Personally I think BEM can be hugely advantageous, especially in a codebase with a lot of people working on it. Probably a conversation to be had elsewhere. Would it be useful to have GitHub Discussions set up on this project? @dessalines @SleeplessOne1917 |
|
Well, I would use BEM here if it would get this merged. This was quite a bit of typing and hopefully not in vain. |
I think more is needed than just converting these classes to BEM. A clear plan would probably have to be laid out. |
I think it would be a good idea to check with the maintainers to make sure something like this is desired before putting this much work into it. |
|
Resubmitted as BEM in the interest of conforming to some actual existing standard |
|
@djvs You seem to be using double dashes for both modifier syntax and child syntax? There are several BEM naming conventions but I'm not familiar with this one. |
|
Whoops, yeah I don't use BEM much these days. Fixing. |
|
👍 |
|
@djvs So, for something like this: 
It looks like these three elements are probably styled very similar, meaning their classes should be modifiers of the same class. So instead of: <label
className="listing-type-select__label-subscribed"
className="listing-type-select__label-local"
className="listing-type-select__label-show-all"
/><label
className="listing-type-select__label listing-type-select__label--subscribed"
className="listing-type-select__label listing-type-select__label--local"
className="listing-type-select__label listing-type-select__label--show-all"
/> |
|
It's these kinds of things that make me think any efforts here need to be part of some larger planning, not hashed out in the comments of a big PR. |
|
That's a fair point, I'll address that now. |
|
I think if and when this project switches to semantic class names, it should be done on a component-by-component basis, not as one big PR that adds a ton of classes that aren't getting styled yet. |
|
I don't think being explicitly styled is a prerequisite to adding classes to a FE that's intended to support themes? Even if so, it'd be better to at least get something down for themers to work off of. Especially with Lemmy userbase taking off right now. |
|
Anyway, probably gonna quit making changes for the moment until maintainers chime in, I feel like this approach is pretty reasonable atm. |
I may be wrong but I don’t think user themes is a high priority right now. The UI needs a lot more work than just the way it looks. Ultimately the main benefit of using semantic class names is maintainability, not for user stylesheets to latch onto. |
|
I applaud the effort but unfortunately I think it may be a bit misguided. I’m not a fan of it. My two cents: If we want to do something like this, we should be using |
|
IDs only for unique elements ( |
|
@djvs @alectrocute @Zetaphor I've written some more about a possible CSS plan in #1371 |
|
Something else I kind of worry about with this is that PRs might start coming in with modifications to |
| @@ -1153,20 +1153,20 @@ export class PostListing extends Component<PostListingProps, PostListingState> { | |||
| ) : ( | |||
| <> | |||
| <button | |||
| className="d-inline-block mr-1 btn btn-link btn-animate text-muted py-0" | |||
| className="post-listing__btn post-listing__btn--transfc-confirm d-inline-block mr-1 btn btn-link btn-animate text-muted py-0" | |||
There was a problem hiding this comment.
transfc = transfer community? The class name's already so long, I see no reason to not just write the rest of the words.
There was a problem hiding this comment.
This is what I mean about planning. Will this specific button really be styled much differently from other buttons? Does it need a modifier?
All these names are chosen with no intention other than making sure everything has a class name on it. These are extremely likely to change, breaking whatever user themes are created in the meantime. Then you have a bunch of user themers getting upset at the project for "breaking" their themes.
This is not what CSS is for.
There was a problem hiding this comment.
@alectrocute It's extremely messy and doesn't serve the larger goals of the project, these are only serving as sledgehammer hooks for custom theming. Obviously this isn't my project, but I just think this whole thing is greatly misguided.
There was a problem hiding this comment.
I agree with you on all counts, @jsit.
There was a problem hiding this comment.
@djvs It seems like the consensus is that this is not a good idea, and that users are going to come complaining to us every time their themes break when we change things around.
Is it so bad to generate bootstrap-compatible themes for site customization?
There was a problem hiding this comment.
That's the product of two constraints:
- Somebody wanting to style that particular button
- Using BEM
And sort of additionally:
- jsit's earlier request to have generic and then modifier substrings for stuff like buttons - `post-listing__btn post-listing__btn--transfc-confirm". Specifically, it's:
Block: post-listing
Element: btn
Modifier: transfc-confirm
The verbosity is my problem with BEM, but without it you are indeed stuck with "confirm-transfc" or whatever. That was basically my initial approach with slightly more namespacing.
There was a problem hiding this comment.
Objectively looked through it and called out any potential issues. I think we need to have further discussion about this.
Mixing BEM and Bootstrap is kinda an abomination. I've never seen anything like it. This will have consequences on the project. As @jsit aptly put, contributors and maintainers will have to follow this practice for every new element we add/change, and will be responsible for it in perpetuity.
|
Well, not my decision to make, but classes/IDs are basically the only way for user styles/scripts to work, and BEM happens to be the prevailing standard for class/ID naming (I'm not a huge fan of it either - you could commit to some modified BEM without double dashes/underscores, but that might be even more trouble). And interest was expressed by 2+ of you in abandoning Bootstrap long-term, in which case this is basically just a step towards that. |
|
P.S. - the kind of bare minimum for people abiding by this standard is "just add a meaningful class name at the root of a component". The additional ones I added were just in the interest of being thorough. |
|
Sorry for taking awhile to review this. I agree with @jsit saying this is a lot of premature optimization. I would be more for it if it wasn't for the fact that contributors will have to come up with these sorts of class names whenever something is added. |
|
In other words, these class names have no actual relationship to how they’re actually going to be used. They’re just there to be there. it’s not “a start” because they’ll likely be abandoned when the codebase switches to Tailwind anyway. AND they will be hard to remove. There are hundreds of new class names here, and none of them has any styling applied to them. To get rid of any one of them, you’d have to know that no styles depend on their being there. |
|
Adding class names without associated styles, just so that they might be custom-styled, is a really good way to pollute your codebase with all kinds of unnecessary markup. It’s asking for trouble. |
|
“Does this class do anything” shouldn’t be a hard question to answer when looking at a codebase. Here, that will be unclear in hundreds of new instances. |
|
OK - can we reach a compromise like "one unique class name per actual component"? I could go either way on BEM, you guys seem split on that. |
|
You can use grep to check if/when a CSS class is used btw. |
|
There might be a bit of "missing the forest for the trees" going on here. At the end of the day the goal is for Lemmy to be something lots of people want to use. For it to be that, it has to look good and be intuitive. That won't be one-size-fits-all in the first place - different visual preferences, devices, browsers, disabilities, you name it. The goal of CSS classes in a context like that isn't simply for the convenience of the developers, it's to allow the UI to support customization. Reddit handled this correctly for years - old.reddit.com was well marked up and styleable (allowing subreddit/community-level styling as well!), and the RES extension took full advantage of this as well. IMO the cost of "having a few classes lying around that don't have associated styles in the main source tree" isn't even on the same order of magnitude. It's actually a bit surprising to encounter this attitude. My reaction to the first opening of Lemmy with F11 inspector was basically "WTF?" - typically sites have at least 25% class coverage for elements. E.g. on this very page - "edit-comment-hide", "timeline-comment-header", "timeline-comment", "unminimized-comment", etc. just in a comment box, along with quite a few others. Paired with utility classes I would add. Sometimes that's minified along with the JS, but not anywhere they expect user customization. I phrase it as "a start" because you could absolutely use my classes as a jumping off point for styling. There's quite a bit of UI markup and logic already - you might want to rethink the class components that Inferno uses if you move to React proper - but no point in rewriting things that are already fine. |
|
Github apparently lets me merge this but no idea what maintainer hierarchy is. I would not like to maintain this indefinitely as a downstream branch, I'm already past the third set of merge conflict resolves. If there's a workable solution here please let me know, I have a lot on my plate right now. (...er, Github does not let me merge it, I'm just used to the "All green checks" display meaning "you can merge") |
|
OK, I won't be able to maintain this branch separately long-term. Can we do a middle of the road compromise like "one meaningful class name per component"? |
|
Simpler version here: |
|
Closing in favor of #1421 |
See #1319. Want to add a lot of classes to allow proper contextualization of CSS.
This should mostly create a migration path away from styles being tied directly to React (well, Inferno) components, which is a big anti-pattern for something that should be themeable.
These are mostly just picked as I went, with a couple small naming conventions that should be apparent. I'm not married to any particular change here besides the general gist of it, so if you want to edit the PR or anything like that let me know.