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.

Sign up for GitHub

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

Jump to bottom

chore(BackgroundImage/AboutModal): updated usage of bg image #8931

Merged
merged 7 commits into from Apr 14, 2023
Merged

chore(BackgroundImage/AboutModal): updated usage of bg image #8931

merged 7 commits into from Apr 14, 2023

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Apr 7, 2023 •
edited

What: Closes #8452

@srambach @tlabaj for AboutModal, the class for the AboutModalBoxHero (.pf-c-about-modal-box__hero) was removed in Core documentation, but still remains in the scss. Would it make sense to remove that sub-component entirely here? There may be a Core followup needed as there is still a CSS var in the scss file that isn't really being used now it seems (--pf-c-about-modal-box__hero--sm--BackgroundImage)

Also Titani, since I'm making updates in LoginPage, would you want me to close out #7867 as well (removing the backgroundImgAlt prop)?

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Apr 7, 2023 •
edited

Preview: https://patternfly-react-pr-8931.surge.sh

A11y report: https://patternfly-react-pr-8931-a11y.surge.sh

nicolethoen
nicolethoen approved these changes Apr 11, 2023
View reviewed changes
packages/react-core/src/components/AboutModal/AboutModal.tsx
@@ -59,7 +61,10 @@ export const AboutModal: React.FunctionComponent<AboutModalProps> = ({
}: AboutModalProps) => {
if (brandImageSrc && !brandImageAlt) {
// eslint-disable-next-line no-console
console.error('AboutModal:', 'brandImageAlt is required when a brandImageSrc is specified, and should not be an empty string.');
console.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@tlabaj
Copy link
Contributor

tlabaj commented Apr 11, 2023

Also Titani, since I'm making updates in LoginPage, would you want me to close out #7867 as well (removing the backgroundImgAlt prop)?

Yes please.

@tlabaj
Copy link
Contributor

tlabaj commented Apr 11, 2023

@srambach @tlabaj for AboutModal, the class for the AboutModalBoxHero (.pf-c-about-modal-box__hero) was removed in Core documentation, but still remains in the scss. Would it make sense to remove that sub-component entirely here? There may be a Core followup needed as there is still a CSS var in the scss file that isn't really being used now it seems (--pf-c-about-modal-box__hero--sm--BackgroundImage)

Unless .pf-c-about-modal-box__hero was unintentionally left out by core, I would say yes, remove the sub-component.

@tlabaj tlabaj requested a review from mcarrano April 11, 2023 17:55
tlabaj
tlabaj requested changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj 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 other than removing the Hero

mcarrano
mcarrano approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member

@mcarrano mcarrano 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 to me. Thanks @thatblindgeye !

This was referenced Apr 12, 2023
Closed
Closed
@thatblindgeye
Copy link
Contributor Author

@tlabaj made the above updates and also created codemod issues for changes made here.

tlabaj
tlabaj approved these changes Apr 12, 2023
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@srambach
Copy link
Member

I'm seeing the close icon smaller than in core. Another svg icon related thing?
image

@tlabaj
Copy link
Contributor

tlabaj commented Apr 14, 2023

I'm seeing the close icon smaller than in core. Another svg icon related thing?

I don't think so @srambach . If you look in my PR that fixes the icons, it looks the same as in this PR.
https://patternfly-react-pr-8954.surge.sh/components/about-modal

srambach
srambach approved these changes Apr 14, 2023
View reviewed changes
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.

I don't actually think the icon is related, this looks fine.

@tlabaj tlabaj merged commit 67f67c5 into patternfly:v5 Apr 14, 2023
10 checks passed
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-code-editor@5.0.0-alpha.71
  • @patternfly/react-core@5.0.0-alpha.70
  • @patternfly/react-docs@6.0.0-alpha.77
  • demo-app-ts@5.0.0-alpha.54
  • @patternfly/react-integration@5.0.0-alpha.25
  • @patternfly/react-table@5.0.0-alpha.72

Thanks for your contribution! 🎉

@tlabaj tlabaj mentioned this pull request Jun 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcarrano mcarrano mcarrano approved these changes

@srambach srambach srambach approved these changes

@nicolethoen nicolethoen nicolethoen approved these changes

@tlabaj tlabaj tlabaj approved these changes

@kmcfaul kmcfaul kmcfaul approved these changes

Assignees
No one assigned
Labels
css review ux review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Login/About modal - update use of background image
7 participants
@thatblindgeye @patternfly-build @tlabaj @srambach @mcarrano @nicolethoen @kmcfaul