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(Progress): prevented empty desc and status from rendering #5387
fix(Progress): prevented empty desc and status from rendering #5387
Conversation
Preview: https://patternfly-pr-5387.surge.sh A11y report: https://patternfly-pr-5387-a11y.surge.sh |
{{#unless progress--no-measure}} | ||
{{> progress-status}} | ||
{{/unless}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only want to remove the status element if there is no measure and no status icon. This update removes the icon from this example, which I don't think we want to do. https://patternfly-pr-5387.surge.sh/components/progress#failure-without-measure
This is the current v4 example that has an empty __status
element - https://pf4.patternfly.org/components/progress#without-measure
Another small thing is that there is a margin on the status icon when there is no measure. What might be better is to turn __status
into display: flex; justify-content: flex-end; align-items: flex-start; gap: var(--whatever-spacer-that-margin-uses)
. That will also remove any space created by whitespace between the measure and status icon, too, and let the gap serve as the only source of space between those elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the styling to __status, and also removed the status-icon__MarginLeft
var since this is a breaking change issue and it would no longer serve any purpose.
The conditional rendering should be working as intended now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! One small formatting change, not a blocker.
{{#if progress--no-measure}} | ||
{{#if (concat progress--danger progress--success progress--warning)}} | ||
{{> progress-status}} | ||
{{/if}} | ||
{{else}} {{> progress-status}} | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
{{#if progress--no-measure}} | |
{{#if (concat progress--danger progress--success progress--warning)}} | |
{{> progress-status}} | |
{{/if}} | |
{{else}} {{> progress-status}} | |
{{/if}} | |
{{#if progress--no-measure}} | |
{{#if (concat progress--danger progress--success progress--warning)}} | |
{{> progress-status}} | |
{{/if}} | |
{{else}} | |
{{> progress-status}} | |
{{/if}} |
🎉 This PR is included in version 5.0.0-alpha.19 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #4328