search

thoughtbot / administrate

A Rails engine that helps you put together a super-flexible admin dashboard.
http://administrate-demo.herokuapp.com
MIT License
5.9k stars 1.12k forks source link

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

Open Uaitt opened 9 months ago

Uaitt commented 9 months ago

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

nickcharlton commented 9 months ago

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 commented 9 months ago

@nickcharlton Yep, I will work on it 😃.

blocknotes commented 4 months ago

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 commented 4 months ago

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

Uaitt commented 4 months ago

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?