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

refactor(about-modal): moved to modal-box #5216

Merged
merged 5 commits into from Nov 10, 2022

Conversation

mattnolting
Copy link
Contributor

closes #4526

@patternfly-build
Copy link

patternfly-build commented Nov 4, 2022

@mcoker mcoker changed the base branch from main to v5 November 7, 2022 22:52
@mcoker mcoker changed the base branch from v5 to main November 7, 2022 22:53
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.

Looks good, have a couple of questions for design.

@mcoker
Copy link
Contributor

mcoker commented Nov 8, 2022

@mattnolting can you rebase this against the v5 branch and update the base branch to v5?

@mattnolting mattnolting changed the base branch from main to v5 November 8, 2022 19:16
@mattnolting mattnolting force-pushed the refactor-about-modal-4526 branch 3 times, most recently from a628d20 to 271032e Compare November 8, 2022 20:04
@mattnolting
Copy link
Contributor Author

@mcoker rebased and ready

@mattnolting mattnolting requested review from mcarrano and mceledonia and removed request for mcarrano November 8, 2022 21:31
@@ -148,7 +128,6 @@
grid-template-columns: var(--pf-c-about-modal-box--lg--grid-template-columns);
grid-template-rows: max-content max-content auto;
max-width: var(--pf-c-about-modal-box--lg--MaxWidth);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max-width: var(--pf-c-about-modal-box--lg--MaxWidth);

looks like you can remove this, it's currently breaking the max-width on lg screens. Although I think this begs the question of whether the about modal should manage its width at all, or if it should just consume the available space and be controlled by the modal. I'd lean toward the latter - wdyt @srambach @mceledonia @mcarrano?

fwiw if you fix this and let the max-width apply, it looks like this in a modal with the modal width set to 50% of the viewport.

Screen Shot 2022-11-09 at 12 24 05 PM

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I completely understand the question @mcoker . What's there now looks good to me. The model should have a max-width IMO. Maybe I'm struggling to differentiate between Modal and About modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcoker updated

Copy link
Contributor

Choose a reason for hiding this comment

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

@mcarrano sorry, it's definitely confusing. The about modal has a custom width and height set, since it is it's own implementation of a modal. With this update, I was proposing we remove the about modal's width, and just let it fill the size of whatever container it's in, which would mean the modal component is used to manage the about modal's width. The modal has width variations for small, medium, large, and a "custom" width.

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.

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.

Nice! Just one small update.

src/patternfly/demos/AboutModal/about-modal-template.hbs Outdated Show resolved Hide resolved
@mcoker mcoker requested a review from srambach November 9, 2022 21:54
@mcoker mcoker merged commit a372934 into patternfly:v5 Nov 10, 2022
@patternfly-build
Copy link

🎉 This PR is included in version 1.2.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@patternfly-build
Copy link

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

The release is available on:

Your semantic-release bot 📦🚀

mattnolting added a commit to mattnolting/patternfly that referenced this pull request Feb 15, 2023
mattnolting added a commit to mattnolting/patternfly that referenced this pull request Feb 15, 2023
mattnolting added a commit to mattnolting/patternfly that referenced this pull request Feb 22, 2023
mattnolting added a commit to mattnolting/patternfly that referenced this pull request May 18, 2023
mattnolting added a commit to mattnolting/patternfly that referenced this pull request Dec 12, 2023
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.

Refactor About Modal to use the Modal component
6 participants