Skip to content

Conversation

@rmleg
Copy link
Contributor

@rmleg rmleg commented May 20, 2022

WHY are these changes introduced?

Fixes #5877

When a page has action groups but no secondary actions, there is not enough spacing above the title of the first action group in the action list popover on mobile.

I can't reproduce the problem in an ActionList in isolation; it only seems to be present in a Page.

WHAT is this pull request doing?

There was a previous PR that fixed this last August by adding a firstSection class to the first section of the action list using the index when mapping across finalSections. However this solution is not reliable as-is because the item at the index of 0 is not always rendered.

Instead of fixing the logic, I decided to go for a CSS solution.

First, set the padding-top for every action list title to the increased amount.

Then, select all titles that are in sections that are not the first section and set their padding-top to the smaller amount.

I had originally done this by just selecting .Section:first-child .Title and setting its padding-top to the increased amount, but that broke the case where an action list is not sectioned (has only one item in its actionGroups array).

Before this PR

The first section has a title which does not have enough spacing above it.

The first section does not have title and all spacing is correct.

After this PR

The first section has a title and it has enough spacing above it.

The first section does not have a title and all spacing is correct. There is no change in this scenario after this PR.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

This can be tophatted by viewing the Page/Page with action groups story in Storybook at a mobile screen size and expanding the action list menu.

You can also verify this does not break the ActionList component in isolation by viewing the Action list/Action list with destructive item and Action list/Sectioned action list stories.

🎩 checklist

@rmleg rmleg force-pushed the rmleg/action-list-first-title branch from a0db23d to 52d5585 Compare May 20, 2022 21:49
@rmleg rmleg requested review from alex-page and chloerice May 20, 2022 21:50
@github-actions
Copy link
Contributor

github-actions bot commented May 20, 2022

size-limit report 📦

Path Size
cjs 151 KB (-0.03% 🔽)
esm 87.37 KB (-0.04% 🔽)
esnext 137.36 KB (-0.04% 🔽)
css 37.3 KB (-0.03% 🔽)

@rmleg rmleg requested a review from laurkim May 20, 2022 22:04
@rmleg rmleg changed the title [Page] Fix spacing above action list title when first in mobile action menu popover WIP [Page] Fix spacing above action list title when first in mobile action menu popover May 20, 2022
@rmleg rmleg marked this pull request as draft May 20, 2022 22:14
@rmleg rmleg force-pushed the rmleg/action-list-first-title branch from 7843438 to 0c7c364 Compare May 20, 2022 22:33
@rmleg rmleg marked this pull request as ready for review May 20, 2022 22:44
@rmleg rmleg requested review from alex-page, chloerice and laurkim May 20, 2022 22:45
@rmleg rmleg changed the title WIP [Page] Fix spacing above action list title when first in mobile action menu popover [ActionList] Fix spacing above action list title when first in mobile action menu popover May 20, 2022
Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

💯

@rmleg rmleg merged commit 42331f3 into main May 23, 2022
@rmleg rmleg deleted the rmleg/action-list-first-title branch May 23, 2022 14:17
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.

[Page] Responsive: Action group title is too close to the top when there are no secondary actions

2 participants