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

fix(global): rename default status color to custom #5418

Merged
merged 4 commits into from Mar 21, 2023

Conversation

mcoker
Copy link
Contributor

@mcoker mcoker commented Mar 8, 2023

fixes #5175

Also makes a status variation required for the alert component. Previously the alert with no variant would give you the "default", aka "custom" status styling. Now the alert without a variation is unstyled and you have to add .pf-m-custom.

@patternfly-build
Copy link

patternfly-build commented Mar 8, 2023

@mcoker mcoker requested review from mattnolting, thatblindgeye and wise-king-sullyman and removed request for mattnolting March 8, 2023 23:04
--pf-c-alert--BorderTopColor: var(--pf-c-alert--m-custom--BorderTopColor);
--pf-c-alert__icon--Color: var(--pf-c-alert--m-custom__icon--Color);
--pf-c-alert__title--Color: var(--pf-c-alert--m-custom__title--Color);
--pf-c-alert--m-inline--BackgroundColor: var(--pf-c-alert--m-inline--m-custom--BackgroundColor);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like --pf-c-alert--m-inline--m-custom--BackgroundColor is defined anywhere

@@ -11,7 +11,7 @@
Danger notification:
{{else if notification-drawer-list-item--IsSuccess}}
Success notification:
{{else if notification-drawer-list-item--IsDefault}}
{{else if notification-drawer-list-item--IsCustom}}
Default notification:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want this updated to "Custom notification:" to follow similar updates in the Alert component?

@@ -4,7 +4,7 @@
{{/if}}>
{{#if popover--IsAlert}}
{{#> screen-reader}}
{{#if popover--IsDefaultAlert}}
{{#if popover--IsCustomAlert}}
Default
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, would we want this text to be "Custom" instead?

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Looks good! Only other thing is the screenshot for the "custom alert popover" example needs to be updated.

@mcoker
Copy link
Contributor Author

mcoker commented Mar 13, 2023

@thatblindgeye great catches! The full-page example screenshots are updated prior to a release, usually with the versioned commit that adds the release notes. Though you can run the job and push to this PR if you'd like! The instructions are at https://github.com/patternfly/patternfly/#update-screenshots Note that it may update more screenshots than this one since that job isn't run with each change and there have been a lot of changes since the last run.

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

@mcoker gotcha! Makes sense to do them all at once then. Everything else looks good!

Copy link
Member

Choose a reason for hiding this comment

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

Line 127 - Should add '.pf-m-custom' as an acceptable modifier (or just remove the list and simply say always to use it with a state modifier)

removed explicit state classes, unbolded that part to match alert and button, which say the same thing.
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 8aa4062 into patternfly:v5 Mar 21, 2023
@patternfly-build
Copy link

🎉 This PR is included in version 5.0.0-alpha.34 🎉

The release is available on:

Your semantic-release bot 📦🚀

mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request May 18, 2023
* fix(global): rename default status color to custom

* chore(text): update default/custom docs

* chore(examples): update example text

* chore(notification-drawer): updated list-item docs

removed explicit state classes, unbolded that part to match alert and button, which say the same thing.
mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Dec 12, 2023
* fix(global): rename default status color to custom

* chore(text): update default/custom docs

* chore(examples): update example text

* chore(notification-drawer): updated list-item docs

removed explicit state classes, unbolded that part to match alert and button, which say the same thing.
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.

Potential namespace conflict with "default" in status prop enums
4 participants