Create front-end development guidelines

[front-endian]

Summary

As a community project I believe it would be useful to develop a set of best practices in an attempt to improve the quality, consistency and ease of working with Sufia's font-end code. This would give developers a place to learn what is expected, would outline many things that should be checked when reviewing pull requests, and could prevent certain discussions from being repeated.

The motivation for this is that while working on the beta interface for U-M's Sufia 7 based repository we found that customization was difficult to perform due to inconsistent & overly complicated views, messy formatting, as well as uneeded & hard to overwrite styles.

Proposed Standards

Note: This is just an initial proposal. I hope we can have an active discussion and revise these until they are something everyone will be happy with an will want to follow. Some of these are on the stricter side, and are opinionated, but Rails is an opinionated framework and the suggestions come from real world issues we have had.

CSS and styling

1. Wherever possible, use pure Bootstrap instead of writing custom CSS.
   • Bootstrap is easier to overwrite than all the custom CSS that was added to Sufia/Curation concerns over the years.
   • Since Bootstrap is only used for styling, developers can know they are safe to removemodify without effecting any JS or tests.
2. CSS selectors should never use id's. They are overly specific and have no practical benefits over classes.
   • A number of years ago most of the front-end community moved away from using ids because of all the long term maintenance problems overly specific selectors cause.
   • Even if the element is unique, a class is still okay and it doesn't suffer from specificity problems.
   • id's should be used for JavaScript because there is a real performance improvement when using them.
3. In any custom CSS, almost all selectors only have one class in them. At most they should use two. Anything more makes it much more difficult and brittle for people to restyle Sufia.
   • If you are unable to do this cleanly, add classes to the markup so you can.
4. Avoid px, use em or rem instead.
   • It is better for accessibility and when done well makes it a lot easier to resize things.
5. Buttons should look like buttons, links should look like links.
   • This means that links need an underline or background-color so that people who are color blind can easily distinguish them from the rest of the text.
6. Links, buttons and inputs must have distinct and easy to see :focus and :hover states.
7. State changes must be shown with an easily noticeable change in brightness or the addition/removal of something like an underline, outline, background-color, etc.
   • Color only state changes are a major accessibility problem.
8. All text should have a contrast ratio of at least 4.5:1 against its background, as defined by WCAG 2.0 A.
9. Class names should describe what the element is, not what the class does.

   Markup and Views

10. Use HTML 5 sectioning elements, just don't over do it.
    • HTML 5 sectioning elements (section, main, article, footer, etc) provide semantic information for assistive technologies, make the markup easier to read, and can improve CSS if they are used in selectors.
    • Be sure to use them appropriately—a little goes a long way. Every page should only have one main and generally should only have one nav element (the latter is design dependent). That main and any article or section elements should only have one header and footer. Avoid nesting article and section tags.
    • Sectioning elements should only highlight the major sections, not every single part of the page.
11. If you have a lot of nested div tags, that is a code smell.
12. If you have a link, make sure it is an a tag, if you something that looks like a button, it must be either an input, button or a tag to preserve semantics and to help assistive technologies.
13. Fewer views are better. There should not be two views that generate near-identical HTML (even if they are pulling from different controllers).
    • Having many small/nested views makes long term maintenance of Sufia harder.
    • Lots of small views does not help front-end developers.
    • It is very hard to make changes to the HTML when you are having to bounce back and forth between 5 nested views. It is a lot easier if you can just find the view for the page or component that you are changing and only have to tweak that one file.
14. Avoid creating a partial unless that HTML is being used multiple times.
    • If someone wants to overwrite a small portion of the HTML they can always edit a little bit of a larger view or create a seperate partial for their own project.
15. Organize the views into folders by how they are related in the interface, not by the controllers that are behind them.
    • A front-end developer shouldn't have to understand how the controllers are structured to alter the views.
    • Presentational and backend concerns should be decoupled.
16. sr-only labels should be avoided as much as possible. If something is important enough that it should be read by someone using a screen reader, it is important enough for everyone to read.
17. No buttons/inputs/links should only consist of an image/icon. There should always be visible text describing what the widget does so all users are helped out.
    • Logos in the the header are one of the few exceptions to this rule.
18. ARIA should be avoided as much as possible.
    • Most of the time semantic HTML is the solution, especially because not all assistive technologies understand ARIA or have all of it implemented.
    • The worst accessibility problems I have had to fix came form when developers used a lot of ARIA and JavaScript trying to make things more accessible.
    • Here is a good article on the subject of less ARIA for better accessibility.
19. Anything added 'for accessibility' must be checked by people knowledgeable about accessibility using a number of the most common assistance technologies before it is released.
    • Accessibility changes that are not tested often result in even worse accessibility problems making things worse for users.
    • Accessibility features that are never audited are like code that has never been run.
20. Even if an element doesn't have a special class applied to it in stock Sufia, make sure there is an easy way to select it, either using the parent element's class or by adding a class to the child element.
    • This will give greater flexibility to institutions so they can modify more things without having to overwrite views
21. Lines of code in a view should be neatly indented and not be longer than about 80 characters long. 100 characters is pushing it, and above 120 should have line breaks to make things more readable.
22. Views shouldn't contain any static text; those should go into the locales config files so they can be translated.

    Related

See #1594 for the start of this discussion

[front-endian]

@grosscol

[jcoyne]

Wherever possible, use pure Bootstrap instead of writing custom CSS.

• Bootstrap is easier to overwrite than all the custom CSS that was added to Sufia/Curation concerns over the years.
• Since Bootstrap is only used for styling, developers can know they are safe to remove modify without effecting any JS or tests.

I'm unclear about what this means and how it would impact the existing user experience. Furthermore, Bootstrap includes a fair amount of Javascript, so I'm not sure that second bullet point is accurate.

[jcoyne]

Avoid px, use em or rem instead.

what about ex, vh/vw, vmax/vmin?

[jcoyne]

Fewer views are better. There should not be two views that generate near-identical HTML (even if they are pulling from different controllers).

• Having many small/nested views makes long term maintenance of Sufia harder.
• Lots of small views does not help front-end developers.
• It is very hard to make changes to the HTML when you are having to bounce back and forth between 5 nested views. It is a lot easier if you can just find the view for the page or component that you are changing and only have to tweak that one file.

I completely disagree on this point. Many small views causes us to remove conditional logic from the views (where it doesn't belong). Small views allow individual institutions to override just the parts they need, which means an easier upgrade path down the road. Regarding "It is very hard to make changes to the HTML when you are having to bounce back and forth between 5 nested views", I think sharing nearly identical views makes this even harder, then you have to test the view partial in multiple different contexts that you may not be aware of.

[jcoyne]

I think points 4 and 5 are the same. Can you merge them?

Organize the views into folders by how they are related in the interface, not by the controllers that are behind them.

The existing organization complies with the Rails standard way of doing things. Moving them into different folders would be unexpected for a Rails developer.

[jcoyne]

ARIA should be avoided as much as possible.

I think you'll have to bring that one up with the accessibility specialists at Penn State and Notre Dame. They (probably) aren't paying attention to this discussion, but they were insistent on getting much of this added.

[jcoyne]

No buttons/inputs/links should only consist of an image/icon. There should always be visible text describing what the widget does so all users are helped out.

• Logos in the the header are one of the few exceptions to this rule.

I think there are a number of very good sites/software that don't follow this recommendation (github comment editor is what I'm looking at now.) So I might have a hard time swallowing this. Too much text is clutter. Icon only buttons can work if used judiciously and accompanied by an aria-label + some sort of tooltip.

[awead]

There are a lot of really big changes here, many of which I'm on-board with. However, it's going to take a lot of work and discussion to get through these. Are we trying to target these changes for Sufia 7? If so, that's going to be difficult. Also, given the breadth of these changes, maybe Github isn't the right place to have this discussion? I'm already lost in this ticket and the it seems the related tickets duplicate points made here.

[mjgiarlo]

@awead No, we're not targeting these for Sufia 7.0.0.

That's the only release we have scheduled/planned right now. Whether the changes @justcolin proposed make it into 7.1, 7.2, or 8.0 depends on how the conversations around the issues @justcolin created (which seem like they're not yet fully resolved, and may be a good candidate for an upcoming Hydra Tech call) go.

[front-endian]

@jcoyne:

• Bootstrap: Using Bootstrap more wouldn't effect users, but it would just reduce reliance on custom CSS. Custom CSS should mostly be used for stylistic overwrites rather than lots of tiny layout tweaks. I am mostly referring to use of Bootstrap for layout. Bootstrap widgets are another story since they do use JS. That being said I think the story is the same: use Bootstrap whenever possible since it is designed to be flexible and modular. The CSS currently in Sufia isn't flexible or very modular, so overwrites are hard. Frameworks are more useful the more fully you adopt them in my experience.
  • We may also want to consider trying to do something more like OOCSS or SMACCS to promote more modular and less brittle custom CSS. @mtribone & @awead: thoughts on this?
• CSS Units: vmin, vh, etc still have some browser support issues and I have yet to see an instance where Sufia would need them. If there is a use case, we should make sure that it gracefully degrades, at least until the browser support story is a little better (it is almost there, but not quiet). ex is fine to use, though I have never seen an instance where anyone has used it since it doesn't really have a practical use that I know of. Most have settled on rem, em and % for the majority of use cases. The use case for px is a really complicated story. The short version is that most have agreed to avoid px when possible to make things better long term. px must be used with care.
• On icon buttons: The fact that some of the most popular websites use icon-only buttons shouldn't be an argument in favor of them since the many if not most of those websites are bad in terms of accessibility. As a tool for institutions we should strive for a higher standard. Icon-only buttons cause a number of problems including (but not limited to):
  • They tend to be very small in both the X and Y axis, making them hard for people with motor disabilities to use, especially on mobile devices. Not only that but bigger and easier to click buttons are easier for everyone to use.
  • Icon-only buttons are more difficult to use for people who aren't as computer literate or aren't familiar with the application. Adding text helps them figure out what is what and do not make anything truly worse for expert users.
  • Icon-only buttons encourage interfaces which are too dense. Not allowing them forces designers to think about how to make their interfaces more open and clearer.
  • We have yet to encounter any interfaces in Sufia or any of our other applications where our designer couldn't figure out how to make a great looking interface that gives enough room to add one word to a button.
  • WYSIWYG editors are one of the exceptions, but that is a very specialized case and WYSIWYG editors tend to be hard for many user groups to use.

[mtribone]

@justcolin I'm interested in OOCSS, SMACCS, or BEM. And there might be a little of BEM in both? I'd like to improve my CSS-fu, so whatever is good for all of us I'll pick up.

As for the vh, vw, etc. units, I believe that I popped it in there when we were using icon fonts to represent a default collection. It is one of many,many tiny tweaks that I made to make Sufia 5-6 a "finished" product for folks. I'm certainly good using em and hope that there aren't a lot of px units littered in the system.

When I look at Sufia 6, I really don't see too many of the icon only buttons. Off of the top of my head, I think there is the trash can for delete and the bull horn for notifications? I don't think text and icon buttons will be an issue because we do a lot of that already in Sufia 6 by using before and after pseudo-elements.

[front-endian]

@mtribone For a project like this I think we should choose something lightweight and easy. Just enough to keep things a little more organized, but not so much that it becomes onerous. It has been a while since I have done a comparison of the options, so I'll look them over again and report back on the pros/cons of each.

One of the problems is that Bootstrap uses px a surprising amount, but Sufia uses them in it's CSS as well. It shouldn't be that hard to strip them out and overwrite the basic font-size rules using em.

Sufia 7 almost always has text to go along with each button, but I'd like to get the "almost always" to "always".

(for @jcoyne and @mjgiarlo as well) We are having a meeting tomorrow to discuss U-M's next steps now that our hacky beta UI is out the door. Once they settle on what direction we will be taking for our UI I should be able to start issuing a lot of small pull requests to work on this issue as well as #1612, #1611 and #1610.

[mjgiarlo]

@justcolin :clap:

[mtribone]

@justcolin This week's reading list

Contextual Styling: UI Components, Nesting, and Implementation Detail http://csswizardry.com/2015/06/contextual-styling-ui-components-nesting-and-implementation-detail/

BEM and SMACSS: Advice From Developers Who’ve Been There http://www.sitepoint.com/bem-smacss-advice-from-developers/

OOCSS/Atomic CSS are Responsive Web Design ‘anti-patterns’ https://benfrain.com/oocss-atomic-css-responsive-web-design-anti-pattern/

[front-endian]

@mtribone: Thanks! I don't think going whole hog with OOCSS, SMACCS, BEM, etc is right for this project, but some more organization is needed to clean things up. That being said, later this week I should be able to come up with a more formal recommendation and write it up here (assuming some other projects don't explode...). I have more reading to do before I can speak with more confidence on this subject.

I am also working on an pull request which show how some example views/CSS can both look better and be easier to customize.

[front-endian]

@mtribone, @mjgiarlo, @jcoyne, @awead and whomever else is interested:

After looking at these for a while I think BEM is the right way to go.

• SMACCS is very well thought out, but it has too many guidelines for Sufia. It takes a while to learn, so unless everyone is already using it, it would be a big barrier to people wanting to submit pull requests for the UI.
• OOCSS is also a nice system, but it is a bit too unstructured. It is more subtle than BEM, meaning that it would be a lot harder to point to something and say definitively why it is wrong. It also doesn't discourage nested selectors as well as SMACCS or BEM.

BEM is light weight and easy to learn, while still providing a lot of structure. The huge benefits we would gain from using BEM is that it would stop all the deeply nested selectors and the use of ID's. This would make restyling and maintenance easier. The other nice thing with BEM is that the documentation is very short and easy to read. You can read about the naming convention here.

Thoughts?

[awead]

Makes sense. Thanks for doing the legwork, @justcolin. My only complaint is cosmetic, in that .form__submit--disabled just looks plain weird to me, but I can get over it. :smile: I think just having some kind of standard is good. Do you know if there are any tools that automatically validate CSS based on this? Unless we really make sure we closely review every PR for CSS compliance, it could be problematic to stick to these rules.

[front-endian]

@awead I also think BEM looks ugly as heck, but it gets the job done.

I do not know of any automated tools to validate this, though it wouldn't be too hard to write one. At the same time, we need every PR request that changes HTML and CSS to be reviewed, else we will end up with the same issues. That being said, these things are very easy and fast for a person to review.

In this case the reviewer just needs to ask the following:

1. Was any new CSS added? If it is doing layout work, the PR author should either refactor to use Bootstrap or justify why Bootstrap can't be used.
2. Are there any selectors that consist of more than one class?
   • If there are any nested selectors or ID's used the author needs to refactor.
   • Almost all selectors should only consist of one class and nothing else.
   • In some instances it is allowable to have one class and one element in a selector, such as something like .tag-group a
3. Look at any HTML changes.
   1. Are there any elements that were added without adding an appropriate selector for people wanting to customize the interface?
   2. Do all new selectors look like BEM selectors?
   3. If there are new BEM blocks in the selectors, are they really new blocks, or should an existing BEM block with a modifier be used?

Because this stuff relies on heuristics and opinions it isn't something that can be fully automated, but we can write a script to flag changes that look suspicious and to be looked at.

[jenlindner]

Interesting to read all of this discussion, and learn the BEM methodology, +1 to it being not overly-structured and getting job done but also ugly at first.

I'm here to add some info from the UX hangout on Sept 27th:

Here are a few links and thoughts on style guides. The first one is about maintaining them (sounds pertinent!), advocates for making components, looks like. http://ianfeather.co.uk/a-maintainable-style-guide/ ianfeather.co.uk

The next one is one I worked with at the Chicago Tribune, and what I appreciated about it was, it was itself an example of the styles it detailed: http://newsapps.github.io/bootstrap/styleguide/ newsapps.github.io

The components in it are also code snippets, and there are small meaningful explanations of why the styles are preferred.

I think with Sufia/CC, our UX efforts, a good goal might be for developers to be familiar with guiding principals to adhere to [like BEM] when making their own UX/UI elements and design decisions.

[mtribone]

Reading back through this thread and thinking about a better reliance on using Bootstrap and more classes instead of nested and overly specific syntax. I came across a code guide by one of the people who works on Bootstrap.

http://codeguide.co/

[mjgiarlo]

@justcolin @mtribone @jcoyne @awead @jenlindner As we start to clarify the paths forward for Sufia and Hyrax, I want to make sure we don't lose the good discussions here. Are there any bits from the above discussion that we should write up as actionable issues?

[mtribone]

@mjgiarlo Yes. I think that we still need to keep @justcolin's original comments about CSS and markup and views in mind when developing. I haven't looked at Hyrax at all, but I've been trying to incorporate his suggestions into every ticket that I work on for Sufia or ScholarSphere.

[front-endian]

@mjgiarlo @mtribone @jcoyne @awead @jenlindner: One suggestion for an actionable bit would be to adopt a CSS/SASS linter like stylelint. I've recently started using it on another project and one of the nicer features is you can have it raise linter warnings when selectors become too complicated or specific.

On an existing project like Sufia you can either configure it from the beginning with really high limits, then slowly crank it down as you fix things, or start it where you want it, add inline disable comments where it fails existing styles, then slowly go through and fix those places.

We have had good success using it to not only act as a linter, but to enforce rules like "only use these units" and "don't use SASS's @extend". You can even configure it so that whenever someone adds an inline comment to turn the linter off for that line they have to add a comment above it explaining why they are making an exception. I like that because it makes makes exceptions more acceptable.

[front-endian]

I'd be more than happy to look into getting it setup to work with Sufia.

[mjgiarlo]

@justcolin @mtribone Thanks, you two! When you make any new PRs or Wiki pages or whatever you find appropriate, it'd be great if you could close this issue. I'll make sure stuff gets carried over to Hyrax.

[mjgiarlo]

(Oops. "Close and comment" is too close to "Comment"...)