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

Doubtful usage of <fieldset> in dashboard show page #2503

Open
Uaitt opened this issue Jan 31, 2024 · 5 comments
Open

Doubtful usage of <fieldset> in dashboard show page #2503

Uaitt opened this issue Jan 31, 2024 · 5 comments
Labels
bug breakages in functionality that is implemented

Comments

@Uaitt
Copy link
Contributor

Uaitt commented Jan 31, 2024

Description

With Administrate v0.20.0, now it's possible to visually group together fields of the same model (introduced in this PR).

While I like the feature, I think that there is a small conceptual problem.

In both the app/views/administrate/application/_form.html.erb and app/view/administrate/application/show.html.erb a <fieldset> element was introduced (indeed, in order to group fields together).
However, most documentation (MDN, W3School, W3 says that this element "groups together element in a form"). With that being said, I really don't think this element should be used in the show view (while I totally agree with using it on the _form partial).

I am writing this also because I have noticed a strange problem on one of my current projects after this update to the <fieldset> in the show view, that occurs when one of the fields contain an horizontal scrollbar.

Show page before the change:

Screenshot 2024-01-31 at 14 38 23
As you see, everything is properly aligned.

Show page after the change:

Screenshot 2024-01-31 at 14 35 59
Layout is completely broken (I had to zoom out a lot in order to see the actual fields). The layout is fixed if the <fieldset> element is replaced with a generic container, like a <div> or <section>.

I would thus propose to replace the <fieldset> element in app/view/administrate/application/show.html.erb with a simple container (either <div> or <section>) and style it similar to a <fieldset>.

Versions

  • Administrate v0.21.0
  • Rails 7.1.3
  • Ruby 3.2.2
@Uaitt Uaitt added the bug breakages in functionality that is implemented label Jan 31, 2024
@Uaitt Uaitt changed the title Doubtful usage of <fieldset> in dashboard show pages Doubtful usage of <fieldset> in dashboard show page Jan 31, 2024
@nickcharlton
Copy link
Member

Oh! Yes, I totally agree.

I do think a <section> is the right thing semantically. Would be able to contribute a PR to fix this?

@Uaitt
Copy link
Contributor Author

Uaitt commented Feb 1, 2024

@nickcharlton Yep, I will work on it 😃.

@blocknotes
Copy link
Contributor

Hey 👋
Any update about this issue?

From my standpoint, the dl tag should have only dt and dd children - also w3 describe the dt tag as:

Content model: Zero or more groups each consisting of one or more dt elements followed by one or more dd elements

I'm asking because it breaks the styles of a theme plugin of mine 😅 and I have to introduce weird styles to handle a fieldset child...

@nickcharlton
Copy link
Member

On #2504, we've got some questions around the semantically correct way to handle this on the show page.

@Uaitt
Copy link
Contributor Author

Uaitt commented Jul 2, 2024

Hi @blocknotes and @nickcharlton, thanks for the follow up on this issue 😃.

Unfortunately, it's been a while since I have last used Administrate and because of that I don't think I can consider myself comfortable enough to continue with the original PR that tried to fix this.

Perhaps @blocknotes you may want to open a PR for the fix yourself and I'll close mine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug breakages in functionality that is implemented
Projects
None yet
Development

No branches or pull requests

3 participants