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

Error text overflow #2432

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Error text overflow #2432

wants to merge 5 commits into from

Conversation

mollykreis
Copy link
Contributor

Pull Request

🀨 Rationale

Fixes #2431

πŸ‘©β€πŸ’» Implementation

The shared error pattern has been updated to use the overflow directive to track when the error text has overflow and to only show the title when overflow is present. As part of this, each component that implements ErrorPattern was updated to include an observable property named errorHasOverflow to correctly implement the ErrorPattern interface.

πŸ§ͺ Testing

Manually tested in storybook that all updated components:

  • don't have a title on the error text when the error text is short
  • have a title on the error text when the error text overflows
  • the overflow on the error text does not affect any existing overflow behavior on the component

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@mollykreis
Copy link
Contributor Author

@m-akinc, will you buddy this PR for me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could/should we replace the ErrorPattern interface with a mixin? Would be nice to remove all the declarations of those three properties and automatically enforce the @attr and @observable expectations.

<div
class="error-text"
${overflow('errorHasOverflow')}
title="${x => (x.errorHasOverflow && x.errorText ? x.errorText : null)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worthwhile to write unit tests for this behavior against one (not all) of the components that use this pattern. I'm pretty sure we have some existing title overflow behavior tests for labels, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error text on components should only have title when content overflows
2 participants