LemmyNet / lemmy-ui

The official web app for lemmy.
https://join-lemmy.org/
GNU Affero General Public License v3.0
880 stars 335 forks source link

Declarative HTML ID naming #863

Open drannex42 opened 1 year ago

drannex42 commented 1 year ago

I am tempted to start a PR on this, unless someone else more well-versed in the lemmy-ui core would prefer taking this over, but:

We need better defining of areas in the HTML scheme, for example:

image This is the bottom navigation bar of a post, this should have an ID or a first-order css class that would define this as post-meta-nav or similar. This does not need to have any attributes attached, just needs something to identify it easier especially for custom themes. Because as it stands, for themes to theme it we have to target the d-flex.justify-content-start.flex-wrap.text.muted.font-weight-bold.mb-1 css selector. This is not operationally coherent or useful, especially if any of those values changes or another element with the same classifer is included.

Comments, navigation bar, thumbnails, avatars, all have this same problem.

For a single theme this would work! But for custom themes, not having any sort of syntactic naming IDs (id="post-meta-nav") really makes theming a chore.

dessalines commented 1 year ago

You should not be building themes targeting specific selectors, you should build a bootstrap v4 theme, and plug it in.. I have a theming guide here: https://join-lemmy.org/docs/en/administration/theming.html

This upcoming year, lemmy-ui will upgrade to bootstrap v5 , and do auto-theme generation from scss files: #703

drannex42 commented 1 year ago

The issue is that allowing specific targeting would allow us to fix a lot of the issues, especially in the layout issues that the current version has for our custom themes.

The current layout decisions are not great for some of themes, for example on beehaw we have the tildes-for-lemmy theme since we love Lemmy, we love the design of Tildes, and we wanted to bring the minimalist look and feel of Tildes along with the capabilities of Lemmy.

drannex42 commented 1 year ago

The lemmy-ui project is great especially since we could build out own front-end, but we wanted to keep up with all the great changes and additions by the Lemmy project with minimal complications.

If we had just simple declarative naming on elements, this would help for both accessibility reasons, userscripts, and for theming. This isn't a just a call for theming, this additionally helps a lot of different users in several different use cases.

dessalines commented 1 year ago

The issue is that allowing specific targeting would allow us to fix a lot of the issues, especially in the layout issues that the current version has for our custom themes.

For specific issues then, lemmy-ui itself should be corrected with the proper bootstrap classes, so that all bootstrap themes can benefit, not just someones' specific custom theme.

Tildes, and we wanted to bring the minimalist look and feel of Tildes along with the capabilities of Lemmy.

Is this an entirely custom classed theme, or is it a bootstrap v4 theme? This is the first time I'm hearing of this theme, and would've preferred someone ask, or consult the theming recommendations before proceeding.

If we had just simple declarative naming on elements, this would help for both accessibility reasons, userscripts, and for theming. This isn't a just a call for theming, this additionally helps a lot of different users in several different use cases.

This really scares me, because then as lemmy-ui gets altered, every single theme that isn't bootstrap compatible or generated will get left in the dust. If I make a custom class or name, then random custom theme makers might ignore that. Using a common framework, with very little custom css, like lemmy-ui does with bootstrap ensures that I don't have to support hundreds of selectors by different theme makers.

Not only that, it ensures that anyone who creates bootstrap themes can use them in lemmy, plug and play.

The accessibility concerns should be addressed in lemmy-ui's code, not in css themes.

drannex42 commented 1 year ago

The accessibility concerns should be addressed in lemmy-ui's code, not in css themes.

That's what I am trying to say, having an ID specified in the HTML syntax is something that is part of the ui code, and helps across the board. Especially for page readers.

Using a common framework, with very little custom css, like lemmy-ui does with bootstrap ensures that I don't have to support hundreds of selectors by different theme makers.

That is on the theme makers, not on you. The declarative addition ensures that things can be selected properly and not forced. As said, use your core bootstrap theming as the base, and then let others who understand that this may need some upkeep on their side to keep their custom themes (that should still be bootstrap based!), well up to date with changes.

Is this an entirely custom classed theme, or is it a bootstrap v4 theme

Bootstrap based (with a lot of generalized hacks, found at the very bottom of the file).

or consult the theming recommendations before proceeding.

I consulted the docs. I realized that this would be a huge task to theme to make it more sensible for our usage (I do not speak for Beehaw, just a user), took quite a bit of time to specific enough without totally mucking up the design of inadvertent elements. The problem is that relying only on abstract CSS classes means really nasty hacks like the one in that file which makes it even harder to keep up to date.

drannex42 commented 1 year ago

I bring all this up, not for Beehaw, or other users of the theme, but as I am about to port all of my companies public forums and internal communications to a Lemmy instance, and we have a style we are trying to get after to work seemlessly with the rest of our system. Some of our users use screen readers and this is something we had to implement in our (current) system. This change would be beneficial for all users, not just us.

dessalines commented 1 year ago

We did an accessibility review a while ago, and added aria labels to and ids most lemmy elements (especially forms), and can add more as necessary. We can add IDs as necessary for screen-readers, but so far we haven't gotten any specific requests for elements. I don't want to add ids to every single html element, as that would be pointlessly overkill, but only as needed.

Also as far as theming goes, I highly recommend you use a bootstrap theme, and whatever it doesn't cover, we can figure out the correct bootstrap way of handling it. Because when we move to bootstrap v5, and subsequent upgrades, all your extra work is probably not going to apply smoothly.

Contrast that with just maintaining a _variables.scss file made with something like https://bootstrap.build , which would then allow all lemmy instances to use that theme.

Klobenda commented 1 year ago

Adding semantic classnames or IDs (where appropriate) throughout the frontend, and adding the ability to use scss (and importing bootstrap scss so that you can use their variables) would enable targeted changes to the front-end without much difficulty.

It's not lemmy-ui's job to maintain third-party themes and it seems more likely that the restrictiveness of the current system would make developers use fragile css selectors that would break mysteriously and be hard to debug if something goes awry.

Some style changes are outright impossible by tweaking the bootstrap vars alone, and the alternative right now is to find out what collection of bootstrap classes an element uses and hoping that nothing else uses a superset of those classes. Adding semantic classnames (possibly a-la BEM syntax, for clarity?) would be a massive boon to theme development.

In general I'm not a fan of using bootstrap classnames. It's barely one step above writing the CSS directly into the style property most of the time, especially with the advent of native CSS vars in the last half decade.

Zetaphor commented 1 year ago

It's probably worthwhile bringing up #1327 which was merged in today. @dessalines and @Klobenda I'd like to get your input on these changes, I only found this thread post-merge.

I agree with the overall goal of keeping things generalized, which is why I felt that the ID's and classes I added were a light-touch approach to enabling more semantic selectors without deeply affecting the markup.

Zetaphor commented 1 year ago

I think this can be closed now that #1327 is merged? @drannex42