Skip to content

feat(min-max-width-height): added min/max height and width to sizing - #4009

Merged
mcoker merged 9 commits into
patternfly:masterfrom
mattnolting:feat-width-limiter-issue-3976
Jun 25, 2021
Merged

feat(min-max-width-height): added min/max height and width to sizing#4009
mcoker merged 9 commits into
patternfly:masterfrom
mattnolting:feat-width-limiter-issue-3976

Conversation

@mattnolting

Copy link
Copy Markdown
Collaborator

closes #3976

@patternfly-build

patternfly-build commented Apr 16, 2021

Copy link
Copy Markdown
Collaborator

Preview: https://patternfly-pr-4009.surge.sh

A11y report: https://patternfly-pr-4009-coverage.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
patternfly-addons.css181.7 kB176.4 kB2.89
utilities/Sizing/sizing.css16.2 kB11.0 kB32.37

@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch from c4d8e2e to 432edc7 Compare April 16, 2021 02:23
@mattnolting
mattnolting requested a review from mcoker May 10, 2021 13:42
@mattnolting

Copy link
Copy Markdown
Collaborator Author

Will update to utility

@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch 2 times, most recently from 4c2ec49 to 5ffb85f Compare May 27, 2021 14:56

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mattnolting wdyt about adding this to the existing sizing utility, basically updating it to support custom width/min-width/max-width using the responsive var stack. We could do the same with height.

@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch from 5ffb85f to 8bb2c21 Compare June 14, 2021 15:29
@mattnolting

Copy link
Copy Markdown
Collaborator Author

@mcoker updated

@mattnolting mattnolting changed the title feat(width-limiter): added width limiter feat(min-max-width-height): added min/max height and width to sizing Jun 14, 2021
@mcoker
mcoker requested a review from srambach June 14, 2021 18:47
@srambach

Copy link
Copy Markdown
Member

I don't understand the page structure and why there are two documentation sections and some parts are "examples" while others are under documentation.
Also, why does it switch to a background color rather than the dashed border we've been using to show the elements?

@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch from a2f20f6 to 0639ce0 Compare June 14, 2021 19:38
@mattnolting

Copy link
Copy Markdown
Collaborator Author

@srambach updated structure and examples


### Usage
| Class | Applied to | Outcome |
| ----------------------------------------------------------- | --------------------- | ----------------------------- |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we relabel "Class" to something like "Custom property"? (same comment applies to line 212)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Nice

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! It's good as is, but left a few comments for review.


### Usage
These utilities are not recommended for general layout purposes. They should primarily be used to enable responsive behaviors of certain components through breakpoints.
| Class | Applied to | Outcome |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit - we typically don't try and align the markdown tables, just use a single space, but it doesn't matter to me.

@@ -0,0 +1,7 @@
<div class="{{#if sizing-min-max--modifier}} {{sizing-min-max--modifier}}{{/if}}"{{#if sizing-min-max--attribute}} {{{sizing-min-max--attribute}}}{{/if}}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit - it's usually written like...

Suggested change
<div class="{{#if sizing-min-max--modifier}} {{sizing-min-max--modifier}}{{/if}}"{{#if sizing-min-max--attribute}} {{{sizing-min-max--attribute}}}{{/if}}>
<div class="{{#if sizing-min-max--modifier}} {{sizing-min-max--modifier}}{{/if}}"
{{#if sizing-min-max--attribute}}
{{{sizing-min-max--attribute}}}
{{/if}}>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

doh

$important: true
);

overflow: auto !important;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I would expect overflow/triggering a scrollbar to be set by default or not. The other sizing classes don't set overflow, and from the utility name, I would expect it to just set that property. Were you thinking it would be best if it did? Scrollable/overflow could also potentially be a utility.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

opened 4147

@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch from 0639ce0 to 616ed57 Compare June 15, 2021 12:16
@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch from 616ed57 to 94fcc3b Compare June 15, 2021 12:19
@mattnolting
mattnolting requested review from mcoker and srambach June 15, 2021 13:55
@mattnolting
mattnolting force-pushed the feat-width-limiter-issue-3976 branch from 786fd92 to c6d2806 Compare June 15, 2021 15:20

@srambach srambach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

L🎃TM!

@mcoker
mcoker merged commit 3b689f6 into patternfly:master Jun 25, 2021
@patternfly-build

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 4.118.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mattnolting
mattnolting deleted the feat-width-limiter-issue-3976 branch January 6, 2022 21:39
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.

Feature: width limiter component

4 participants