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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace <fieldset> with <section> on show pages #2504

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

Conversation

Uaitt
Copy link
Contributor

@Uaitt Uaitt commented Feb 1, 2024

This PR fixes issue #2503.

Basically, the <fieldset>s and <legend>s in

  • app/views/administrate/application/show.html.erb
  • app/views/fields/has_one/_show.html.erb

have been replaced by a combination of <section>s, <h1>s and <nav>s. Those new elements have then been styled so that they look exactly like the previous two.

A few screenshots down here 馃憞

BEFORE THIS CHANGE:

Show page of an object with some grouped and ungrouped attributes.
Screenshot 2024-02-01 at 16 57 28

Show page of an object with an has_one field and some other attributes. (Notice the overlapping of borders).
Screenshot 2024-02-01 at 16 57 35

AFTER THIS CHANGE:

Show page of an object with some grouped and ungrouped attributes. (Note the difference in width of the grouped block).
Screenshot 2024-02-01 at 16 55 51

Show page of an object with an has_one field and some other attributes. (Note that there's no more overlapping).
Screenshot 2024-02-01 at 16 55 40

Any feedback is welcomed 馃槂.

PLEASE NOTE that I am not a frontend dev, if you notice something very stupid or inappropriate in this PR don't be afraid of telling me 馃槃.

@Uaitt Uaitt marked this pull request as draft February 1, 2024 16:43
@Uaitt Uaitt force-pushed the replace-fieldsets-on-show-pages branch from 6b001a5 to 138af76 Compare February 1, 2024 16:46
@Uaitt Uaitt marked this pull request as ready for review February 1, 2024 16:51
@@ -43,9 +43,13 @@ as well as a link to its edit page.
<section class="main-content__body">
<dl>
<% page.attributes.each do |title, attributes| %>
<fieldset class="<%= "field-unit--nested" if title.present? %>">
<section class="<%= "field-unit--nested" if title.present? %>">
Copy link
Contributor

Choose a reason for hiding this comment

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

Semantically, <fieldset> elements typically wrap interactive form controls. Since the show templates present information without the opportunity to edit that information, this change improves the semantics of what these elements are communicating.

Having acknowledged that, rendering multiple <h1> elements poses some accessibility risks, and nesting a <nav> element (a role that comes with its own set of semantics and requirements) inside an <h1> poses additional risks.

Are there other structural changes we could make to achieve a similar outcome without the risks?

Copy link
Contributor Author

@Uaitt Uaitt Feb 8, 2024

Choose a reason for hiding this comment

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

@seanpdoyle what element (or elements) would you recommend to replace my <h1> and <nav> combination? Do you have something in particular in mind? 馃

@nickcharlton nickcharlton added the views-and-styles how administrate looks and is interacted with label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
views-and-styles how administrate looks and is interacted with
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants