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

Change .dl-horizontal to be a grid #1077

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Change .dl-horizontal to be a grid #1077

merged 2 commits into from
Dec 9, 2024

Conversation

matthew-white
Copy link
Member

@matthew-white matthew-white commented Dec 5, 2024

We have two types of <dl> elements in Frontend:

  1. <dl> elements that appear in SubmissionBasicDetails, EntityBasicDetails, and EntityData. The <dt> comes before the <dd> in the DOM, but for display, the <dt> is shown below the <dd>. We use flexbox and flex-direction: column-reverse to achieve that.
  2. The <dl> element in EntityUploadHeaderErrors. This uses the dl-horizontal class in order to display the <dt> elements and the <dd> elements as two columns.

In hover cards, we're going to be using <dl class="dl-horizontal">. However, there's an issue: right now, <dt> elements in .dl-horizontal have a fixed width of 160px. Working on hover cards, I've found that different hover cards need different widths for their <dt> elements. We want the <dt> elements to be as narrow as possible, since hover cards as a whole are pretty narrow. Yet in some cases, <dt> elements need to be fairly wide, e.g., "Last Submission" in the hover card for a form is pretty wide. I want to set it up so that <dt> elements are as wide as they need to be, but no wider. That'll leave the rest of the width of the hover card to the <dd>, whose text is more likely to be arbitrarily long. I also think that strategy will work great with i18n, since some translated text is likely to be longer than the English. We really want to avoid truncating <dt> elements in hover cards, because it's not possible to hover over a hover card in order to view a tooltip. There's more to say about the width of hover cards and the <dt> elements within them, but the general point is that we need the width of <dt> elements to be more dynamic than they are now.

What has been done to verify that this works as intended?

This PR only touches styles. I took a look locally at both types of <dl> elements.

Why is this the best possible solution? Were any other approaches considered?

Setting max-width on the <dt> elements is not enough, because each pair of <dt>/<dd> elements is independent of the others. We need the width of every <dt> element to be the same, since they're displayed like a column. To achieve that, I've changed <dl class="dl-horizontal"> to use a CSS grid. That allows us to display the <dt> elements like a column while also giving us a lot of flexibility around the width of the elements.

One requirement of the CSS grid (I think) is that the children of <dl class="dl-horizontal"> need to be <dt> and <dd> elements, not <div> elements. Those <div> elements are needed for the flexbox used in the first type of <dl> above, but they aren't needed for (and I think don't work with) a CSS grid. In this PR, I've removed <div> elements in EntityUploadHeaderErrors.

I didn't try to change the first type of <dl> to use a CSS grid instead of flexbox. I feel like that type of <dl> is served well by flexbox. I've added comments about the structure that each type of <dl> expects.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

In this PR, I've refactored .dl-horizontal to use a CSS grid, but I've tried to keep its current behavior the same: EntityUploadHeaderErrors should not have changed at all. The goal of this PR is to set the groundwork for the more dynamic behavior that we'll need in hover cards.

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@matthew-white matthew-white requested a review from ktuite December 5, 2024 21:44
@matthew-white
Copy link
Member Author

matthew-white commented Dec 9, 2024

I've made a few smaller changes after code review. I don't think they require a re-review, but I wanted to mention them:

  • In .dl-horizontal, I reverted the space between the <dt> and the <dd> back to 20px. I'll wait until #1067 to make it 24px. I ended up thinking that it makes sense for this PR to be a pure refactor, then I can make any user-facing changes needed for hover cards later.
  • I removed the new .dl-horizontal-fixed class and reverted the width of the <dt> back from fit-content(50%) to 160px (+ padding). I'll reintroduce fit-content() and max-content as part of #1067. When I use max-content with hover cards, I want to make the <dt> and the <dd> the same width, but that'll require some JavaScript. I think that fits better in #1067: this PR is just CSS.
  • I took a closer look locally at the EntityUploadHeaderErrors component, and there were a few small styling issues that this PR had introduced that I fixed.

@matthew-white matthew-white merged commit 55217ea into master Dec 9, 2024
1 check passed
@matthew-white matthew-white deleted the dl-grid branch December 9, 2024 03:20
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.

2 participants