Skip to content

fix(Pagination): updated width logic for page selection#6276

Merged
mcoker merged 2 commits intopatternfly:v6from
thatblindgeye:iss6236_paginationInputCutoff
Feb 2, 2024
Merged

fix(Pagination): updated width logic for page selection#6276
mcoker merged 2 commits intopatternfly:v6from
thatblindgeye:iss6236_paginationInputCutoff

Conversation

@thatblindgeye
Copy link
Contributor

Closes #6236

--#{$pagination}__nav-page-select--FontSize: var(--pf-t--global--font--size--body--md);
--#{$pagination}__nav-page-select--ColumnGap: var(--pf-t--global--spacer--sm);
--#{$pagination}__nav-page-select--c-form-control--width-base: calc(var(--pf-t--global--spacer--sm) * 2 + var(--pf-t--global--border--width--100) * 2); // TODO: replace border width token with semantic token
--#{$pagination}__nav-page-select--c-form-control--width-base: calc((var(--pf-t--global--spacer--md) * 2) + (var(--pf-t--global--border--width--control--default) * 2));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously we were using a small 8px padding on the actual form control input, now we're using medium 16px padding inline start/end. Also update the border token.

Should we instead use the actual form control vars here, though? If a consumer updates the form control input padding, it'll cause the same cutoff issue unless they also update vars in Pagination.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we instead use the actual form control vars here, though?

@thatblindgeye yeah, I think that would be ideal. Same deal if we update that padding or border or whatever - we'll have to update any other components that do this sort of thing.

Along those lines, looks like we support this in a handful of components. IMO this would be much more useful as logic in the form control's text input. Then we can yank all of that logic out of the other components and those components can just set a --[pf form control]--chars var for this. Here's where all I'm seeing it, though there may be other places I missed.

--#{$date-picker}__input--c-form-control--Width: calc(var(--#{$date-picker}__input--c-form-control--width-chars) * 1ch + var(--#{$date-picker}__input--c-form-control--width-base));
--#{$date-picker}__input--c-form-control--width-base: calc(var(--#{$pf-global}--spacer--xl) + var(--#{$pf-global}--spacer--sm)); // form control left/right padding + status icon width and spacer
--#{$date-picker}__input--c-form-control--width-chars: 10;

--#{$number-input}--c-form-control--width-base: calc(var(--pf-t--global--spacer--md) * 2 + var(--pf-t--global--border--width--box--default) * 2); // form controls's padding
--#{$number-input}--c-form-control--width-icon: var(--pf-t--global--spacer--xl); // extra width to accommodate a status icon
--#{$number-input}--c-form-control--width-chars: 4;
--#{$number-input}--c-form-control--Width:
calc(
calc(
var(--#{$number-input}--c-form-control--width-base) + var(--#{$number-input}--c-form-control--width-chars) * 1ch
) + var(--#{$number-input}--c-form-control--width-icon)
);
}

--#{$slider}__value--c-form-control--width-base: calc(var(--#{$form-control}--PaddingLeft) + var(--#{$form-control}--PaddingRight) + #{pf-size-prem(20px)}); // form control base width + number input spinner width
--#{$slider}__value--c-form-control--width-chars: 3;
--#{$slider}__value--c-form-control--Width: calc(var(--#{$slider}__value--c-form-control--width-base) + var(--#{$slider}__value--c-form-control--width-chars) * 1ch);

That could be a follow-up or you could try that now and see if it doesn't open a can of worms, up to you. My guess is it should be pretty straight forward.

Copy link
Contributor Author

@thatblindgeye thatblindgeye Feb 2, 2024

Choose a reason for hiding this comment

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

@mcoker for now I just updated Pagination, wasn't 100% sure how to go about pulling some of the logic out of the other components into form control. What I had thought was in form control, updating the Width var to:

--#{$form-control}--Width: calc((var(--#{$form-control}--width-chars, 0) * 1ch) + var(--#{$form-control}--width-base, 100%));

So that we end up 100% width by default like we currently have, then in other components we'd need to at least define vars to assign to --#{$form-control}--width-chars and --#{$form-control}--width-base, since other components will need different values for both of those vars (could maybe define a var in FormControl that = its padding left + padding right then use that var in other components, e.g. --component--c-form-control--width-base: var(--#{$form-control}--padding-inline-base) + [whatever additional units we need to take into account like border width, icon width, etc])

But not sure how off the mark that would be 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Good stuff! Sounds like a fun problem for future us to solve 🐸

@patternfly-build
Copy link
Collaborator

patternfly-build commented Feb 2, 2024

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Left a comment, but LGTM as it is now, too! 👍

Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson 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 530958a into patternfly:v6 Feb 2, 2024
@patternfly-build
Copy link
Collaborator

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

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