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: Fix for title size issue. #351

Merged
merged 2 commits into from
Dec 4, 2024
Merged

fix: Fix for title size issue. #351

merged 2 commits into from
Dec 4, 2024

Conversation

dlabaj
Copy link
Contributor

@dlabaj dlabaj commented Nov 26, 2024

Fixes issue #342 by removing the extra title tag.

Copy link

netlify bot commented Nov 26, 2024

Deploy Preview for quickstarts ready!

Name Link
🔨 Latest commit 921cdab
🔍 Latest deploy log https://app.netlify.com/sites/quickstarts/deploys/674fcb4253d6d00008ac04b4
😎 Deploy Preview https://deploy-preview-351--quickstarts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dlabaj dlabaj linked an issue Nov 26, 2024 that may be closed by this pull request
fix: removed title.
Copy link
Contributor

@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.

image

Titles look good. I noticed the drawer close button should be icon--color--on-brand--default.

Other than that looks good! Thanks Don

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

lgtm. Approving if we want to do the close button update in a follow up, or will re-review if this PR is updated

Copy link
Member

@evwilkin evwilkin left a comment

Choose a reason for hiding this comment

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

LGTM for the font size, but calling out that this changes the DOM which could break user tests?

@dlabaj
Copy link
Contributor Author

dlabaj commented Dec 4, 2024

LGTM for the font size, but calling out that this changes the DOM which could break user tests?

Yes there's a chance this could break users testing. We discussed this and talked about if we should try to not modify the DOM. However after discussing the issue, we decided that we should go ahead with this fix since it was incorrect to begin with, and a fix that would not modify the DOM wouldn't be the correct solution.

@dlabaj
Copy link
Contributor Author

dlabaj commented Dec 4, 2024

Titles look good. I noticed the drawer close button should be icon--color--on-brand--default.

Other than that looks good! Thanks Don

All set @andrew-ronaldson
Screenshot 2024-12-03 at 10 26 21 PM

@dlabaj dlabaj merged commit 2792698 into patternfly:main Dec 4, 2024
9 checks passed
Copy link

github-actions bot commented Dec 4, 2024

🎉 This PR is included in version 6.1.0-prerelease.4 🎉

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.

Card title size doesn't match cards component
5 participants