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

fix(calcite-loader, calcite-input-message)!: drop active in favor of hidden #5761

Merged
merged 13 commits into from
Nov 17, 2022
Merged

fix(calcite-loader, calcite-input-message)!: drop active in favor of hidden #5761

merged 13 commits into from
Nov 17, 2022

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Nov 15, 2022 •
edited
Loading

Related Issue: #3821

Summary

Loader and Input-Message are small building blocks that an owning component should be able to display or hide.

This deprecates/removes the role of active prop in toggling visibility in favor of the global attribute hidden. Styles for hidden take precedence over active.

The benefit is one less custom prop to manage, and it aligns with existing HTML conventions.

Removes active knob from stories and subs the ones set to false with static hidden for consistency, as we don't provide knobs for native global attributes.

@Elijbet Elijbet changed the title Elijbet/3821 loader/input message drop active to favor hidden breaking fix(calcite-loader, calcite-input-message)!: drop active in favor of hidden Nov 15, 2022
@Elijbet Elijbet added the breaking change Issues and pull requests with code changes that are not backwards compatible. label Nov 15, 2022
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Nov 15, 2022
@Elijbet Elijbet marked this pull request as ready for review November 15, 2022 22:20
@Elijbet Elijbet requested a review from a team as a code owner November 15, 2022 22:20
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Nov 15, 2022
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 16, 2022
@Elijbet
Remove active knob from stories and sub the ones set to false with st…
2bbe345 
…atic hidden for consistency, as we don't provide knobs for native global attributes
driskull
driskull approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

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

I think just stories need to be updated with regards to toggling hidden in some cases.

We also should decide if we need to document the global hidden attribute.

src/components/inline-editable/inline-editable.stories.ts Outdated Show resolved Hide resolved
src/components/input-number/input-number.stories.ts Outdated Show resolved Hide resolved
src/components/input-text/input-text.stories.ts Outdated Show resolved Hide resolved
src/components/input/input.stories.ts Outdated Show resolved Hide resolved
src/components/loader/readme.md Outdated Show resolved Hide resolved
jcfranco
jcfranco reviewed Nov 16, 2022
View reviewed changes
src/components/inline-editable/inline-editable.stories.ts Outdated Show resolved Hide resolved
src/components/input-number/input-number.stories.ts Outdated Show resolved Hide resolved
src/components/loader/readme.md Outdated Show resolved Hide resolved
src/components/loader/loader.e2e.ts Outdated Show resolved Hide resolved
src/components/loader/loader.tsx Show resolved Hide resolved
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Nov 17, 2022
driskull
driskull approved these changes Nov 17, 2022
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

🎉 🌮

@Elijbet Elijbet merged commit c2e05d1 into master Nov 17, 2022
@Elijbet Elijbet deleted the elijbet/3821-loader/input-message-drop-active-to-favor-hidden-breaking branch November 17, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco left review comments

@driskull driskull driskull approved these changes

Assignees

@Elijbet Elijbet

Labels
breaking change Issues and pull requests with code changes that are not backwards compatible. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
Sprint 2022/11/07 - 2022/11/18
Development

Successfully merging this pull request may close these issues.
3 participants
@Elijbet @jcfranco @driskull