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

refactor(generic): improve action buttons #696

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

koeaw
Copy link
Contributor

@koeaw koeaw commented Mar 4, 2024

This is a first attempt at improving the action buttons. Can be split into smaller PRs if that's better.

What this currently does is:

  • replace hardcoded CSS for relevant elements (a, td) with classes to allow easier overriding,
  • change how the icons (and alternative text) are integrated into the button links -> accessibility; makes buttons tabbable like all other links more nicely,
  • change the colour scheme to make more logical sense -> use regular link colour for "view" button because it's the most neutral (it doesn't "do" anything); switch "duplicate" button in apis_entities to green because it actually adds new content,
  • adapt color of buttons for accessibility -> use colour contrast that is WCAG AAA compliant.

If we offered a way to switch the accessibility level of the APIS interface (haven't seen them in a while, but know that gov websites used to have e.g. buttons labelled "AA", "AAA"), the "duller" looking colours could be kept in separate stylesheets. I'm actually assuming there are projects which have to fulfil certain accessibility criteria due to how they are funded (?!), so I'm wondering if we shouldn't actually develop for accessibility out of the box.

- adapt use of icons vs. text for accessibility
- move colors to static file to allow overriding
- switch to color variables from Bootstrap "context" keywords
- replace hardcoded CSS for table cells with class
@koeaw
Copy link
Contributor Author

koeaw commented Mar 4, 2024

Before:
Screenshot from 2024-03-04 15-23-53_cropped

After:
Screenshot from 2024-03-04 15-20-43_cropped

@b1rger
Copy link
Contributor

b1rger commented Mar 4, 2024

In my test setup the buttons now look like this:
Screenshot-2024-03-04-152138

If we change the color of elements - wouldn't it make more sense to do this for the whole bootstrap theme and not for individual elements?

@koeaw
Copy link
Contributor Author

koeaw commented Mar 4, 2024

If we change the color of elements - wouldn't it make more sense to do this for the whole bootstrap theme and not for individual elements?

In theory yes, agree. But I don't think anyone has the resources to do that. I certainly don't, so would opt for doing this bit by bit.

E.g. there are some defaults we don't explicitly set, which would need changing, like the main link colour. But then there are also these variable keywords that Bootstrap provides ("warning" etc.), which we could ignore in favour of custom variables (setup of which would require more frontend work).

The colour contrast thing I did here is just a little thing, colours in general would also need checking against e.g. red-green colour blindness. And while I was looking at the button colours against the (striped) table, which is meant to make the rows easier to differentiate, I noticed that the stripes aren't implemented in the best way, either. Doh!

And e.g. ARIA attributes are their own (huge) topic again. I haven't looked into how well (or not) Bootstrap supports them (that is, out of the box).

@koeaw koeaw marked this pull request as ready for review March 4, 2024 16:26
@koeaw
Copy link
Contributor Author

koeaw commented Mar 4, 2024

I wasn't actually planning to add more to this, so un-drafting. But... should I split this into smaller PRs?

Idk, maybe one just for the switch from hard-coded stuff to CSS classes that can be overridden? Then another one for the proposed changes to the current CSS?

@koeaw
Copy link
Contributor Author

koeaw commented Mar 4, 2024

Also, not sure sure if the changes in generic should be kept separate from apis_entities building on top of them (with the duplicate button).

@b1rger
Copy link
Contributor

b1rger commented Mar 4, 2024

I'd propose the following:

  • on PR looks fine to me
  • only one css file in core/static
  • use the default colors of bootstrap for now
  • use a separate css file with the contrast adapted and the colors changed - I usually do these adaptions in my app until they are "grown" enough to be added to the core repo, but I'm also happy if you want that file in the core repo now, though I'm skeptical about enabling them because then we would have two concurrent color themes enabled at the same time. The beauty of Django is that you can simply drop that file in your own static folder and add a
{% block styles %}
  {{ block.super }}
  <link rel="stylesheet" href="{% static 'columns/mycustom.css' %} " />
{% endblock styles %}

to use it

@koeaw
Copy link
Contributor Author

koeaw commented Mar 4, 2024

Tbh, I couldn't quite figure out how either styles or scripts are supposed to be added – I saw the blocks for styles and scripts in base.html but found the overall file hard to parse/understand, esp. in terms of how the rest of the stylesheets/scripts get included. Also got confused by those *.html partials that only seem to serve to include links to stylesheets, IIRC. (?)

Side note: this reminds me I actually already moved a few things into separate .css and .js files, which I stashed somewhere on my end (example: that scroll-to-top button). I ultimately didn't know where/how best to save them, or how separate and independent the individual Core apps should be kept. Either way, IMO it's way preferable to have the html, css and js separated out. Having it all contained within/spread out over individual templates makes it harder to track down (to debug, refactor, add to,...).

Ok, so one css file where we collect everything together for now. Any naming preferences? I've seen styles.css (corr: plural) used as example in several places. (No generic equivalent for JavaScript, though.)

@koeaw
Copy link
Contributor Author

koeaw commented Mar 4, 2024

Maybe core.css, core.js. As slightly more app-specific version of generic ones like main.css, main.js. (I'm only mentioning JS because I'm thinking ahead. Wanna turn my stash into a PR.).

@b1rger
Copy link
Contributor

b1rger commented Mar 5, 2024

I'd call it apis.css

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.

None yet

2 participants