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

Remove ordered list markup for organisation logos #1860

Merged
merged 2 commits into from
Sep 22, 2020

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Sep 16, 2020

⚠️ This application is Continuously Deployed: ⚠️

  • Merged changes are automatically deployed to staging and production.

  • Make sure you follow the guidance for deployments before you merge.

  • Check your branch is being deployed in the Release app, after merging.

What

Move logos at the top of organisation pages out of the ordered list element.

Why

These logos should not be in an ordered list as there's nothing to denote them as ordered. This could've been changed into an unordered list but this itself isn't ideal as they aren't really intended as a list. Example of a page with multiple logos.

No visual changes.

Card: https://trello.com/c/zeNDQ9hA/374-ordered-list-used-when-list-is-not-ordered

@bevanloon bevanloon temporarily deployed to government-f-remove-ord-xi8oki September 16, 2020 08:54 Inactive
@maxgds
Copy link
Contributor

maxgds commented Sep 17, 2020

Can you link to some example pages? Particularly interesting would be ones with more than one logo.

@owenatgov owenatgov force-pushed the remove-ordered-list-around-logos branch from 3bf1db8 to f92fd3a Compare September 17, 2020 14:48
@bevanloon bevanloon temporarily deployed to government-f-remove-ord-xi8oki September 17, 2020 14:48 Inactive
@owenatgov owenatgov requested a review from maxgds September 17, 2020 14:51
@danacotoran
Copy link
Contributor

@owenatgov this may very well be a daft question but why do you say they are they not intended to be a list?

Isn't the purpose of that markup to list the organisations that own that particular document/publication/page/whathaveyou?
I can get behind the argument that it does not particularly make sense to have them as an <ol> as there's nothing ordered about it, but I'm not sure I understand why it shouldn't be a <ul> 🤔

@owenatgov
Copy link
Contributor Author

@danacotoran So in short, I don't really have an explanation. I was on the fence about if it should be a ul or not. When I took the ol out I also thought it made sense to de-list it altogether because when I observed this it felt like improper use of list semantics because it wasn't a traditional list aka: items "listed" one after the other. However, I also wondered if this perspective came from a sighted bias and, removing the visual context, this is perfectly acceptable as a list. I feel like your reasoning makes perfect sense, I'll swap this out for a ul instead.

@owenatgov owenatgov force-pushed the remove-ordered-list-around-logos branch from f92fd3a to f4253a2 Compare September 18, 2020 17:05
@bevanloon bevanloon temporarily deployed to government-f-remove-ord-xi8oki September 18, 2020 17:05 Inactive
Copy link
Contributor

@danacotoran danacotoran left a comment

Choose a reason for hiding this comment

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

👍🏼

@injms injms merged commit fa2285e into master Sep 22, 2020
@injms injms deleted the remove-ordered-list-around-logos branch September 22, 2020 13:05
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.

5 participants