8000 fix(vars): clean up references to old button spacers by mcoker · Pull Request #6851 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

fix(vars): clean up references to old button spacers#6851

Merged
mcoker merged 3 commits intopatternfly:v6from
mcoker:issue-6640
Jul 18, 2024
Merged

fix(vars): clean up references to old button spacers#6851
mcoker merged 3 commits intopatternfly:v6from
mcoker:issue-6640

Conversation

@mcoker
Copy link
Contributor
@mcoker mcoker commented Jul 3, 2024

fixes #6640

Comment on lines +101 to +102
--#{$data-list}__item-action__action--MarginBlockStart: calc(var(--pf-t--global--spacer--sm) * -1); // former form-element
--#{$data-list}__action--MarginBlockStart: var(--#{$data-list}__item-action__action--MarginBlockStart); // update at breaking change
--#{$data-list}__item-action__action--MarginBlockEnd: calc(var(--pf-t--global--spacer--sm) * -1); // former form-element
--#{$data-list}__item-action__action--MarginBlockStart: calc(var(--pf-t--global--spacer--control--vertical--plain) * -1);
--#{$data-list}__item-action__action--MarginBlockEnd: calc(var(--pf-t--global--spacer--control--vertical--plain) * -1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +336 to +335
margin-block-start: var(--#{$data-list}__action--MarginBlockStart);
margin-block-start: var(--#{$data-list}__item-action__action--MarginBlockEnd);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Should this be --MarginBlockStart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! Updated and updated a > *:not(:last-child) inline margin to gap - 66af31d

@patternfly-build
Copy link
Collaborator
patternfly-build commented Jul 3, 2024

--#{$table}--m-grid__check--favorite--MarginInlineStart: var(--pf-t--global--spacer--xl);

// * Table grid action
--#{$table}--m-grid__action--MarginBlockStart: #{pf-size-prem(6px)};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't look necessary:

Before
Screenshot 2024-07-03 at 3 57 41 PM

After. The cell's top padding matches the button's, so no margin on the action wrapper aligns the button text with the cell text
Screenshot 2024-07-03 at 3 57 58 PM

Comment on lines +295 to +296
margin-block-start: var(--#{$table}--m-tree-view-grid__action--MarginBlockStart);
margin-block-end: var(--#{$table}--m-tree-view-grid__action--MarginBlockEnd);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This moves the margin to the actions container since anything can go in the __action cell (not just a menu-toggle or button). @mattnolting do you see any problem with the margin on __action? It also has a bunch of padding and stuff but aligns fine as far as I can tell.

@mcoker mcoker requested review from mattnolting and srambach July 3, 2024 22:51
--#{$data-list}__action--MarginBlockStart: var(--#{$data-list}__item-action__action--MarginBlockStart); // update at breaking change
--#{$data-list}__item-action__action--MarginBlockEnd: calc(var(--pf-t--global--spacer--sm) * -1); // former form-element
--#{$data-list}__item-action__action--MarginBlockStart: calc(var(--pf-t--global--spacer--control--vertical--plain) * -1);
--#{$data-list}__item-action__action--MarginBlockEnd: calc(var(--pf-t--global--spacer--control--vertical--plain) * -1);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like --#{$data-list}__action--MarginBlockStart is set in the compact variant on line 155 but never used.

Copy link
Member

Choose a reason for hiding this comment

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

(I think you can remove line 155?)

Comment on lines +336 to +335
margin-block-start: var(--#{$data-list}__action--MarginBlockStart);
margin-block-start: var(--#{$data-list}__item-action__action--MarginBlockEnd);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be --MarginBlockStart?

@mcoker
Copy link
Contributor Author
mcoker commented Jul 10, 2024

@srambach updated, changed the __item-action__action vars to __action after moving the .#{$data-list}__action {} block to the root instead of being nested in .#{$data-list}__item-action__action {}

Backstop report reports no failures compared to the last backstop run - backstop-data-list-7.10.24.pdf

Here's the commit - 71d74e2

@andrew-ronaldson andrew-ronaldson added this to the Penta final release milestone Jul 16, 2024
@mcoker mcoker requested a review from srambach July 17, 2024 20:43
Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

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

🐵 LGTM

@mcoker mcoker merged commit d4a0a23 into patternfly:v6 4D2B Jul 18, 2024
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.0.0-alpha.196 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0