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

[calcite-loader, calcite-input-message] Drop active in favor of hidden #3821

Closed
jcfranco opened this issue Dec 30, 2021 · 10 comments
Closed
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. refactor Issues tied to code that needs to be significantly reworked.

Comments

@jcfranco
Copy link
Member

jcfranco commented Dec 30, 2021

Description

Stems from https://github.com/Esri/calcite-components/pull/3778/files#r774706383

It seems that calcite-loader/calcite-input-message[active] is used to toggle visibility, so we should deprecate/remove it in favor of the global hidden attribute.

https://codepen.io/jcfranco/pen/KKXQErV?editors=1000

To avoid breaking changes:

  • active should be deprecated
  • styles for hidden should be added (these have higher precedence than active)

Proposed Advantages

One less custom prop to manage, and it aligns with existing HTML conventions.

Relevant Info

N/A

Which Component

  • calcite-loader
  • calcite-input-message
@jcfranco jcfranco added discussion Issues relating to a conversation where feedback is optional. design Issues that need design consultation prior to development. refactor Issues tied to code that needs to be significantly reworked. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Dec 30, 2021
@driskull
Copy link
Member

Agreed.

We do have some components that are not visible by default...

  • modal
  • popover
  • tooltip
  • ?

However, these use visibility to hide them instead of display. Also, we would expect those components to not be visible by default. In this case, I think I would expect loader to be visible by default.

@benelan benelan removed the needs triage Planning workflow - pending design/dev review. label Dec 30, 2021
@benelan benelan added this to the Freezer milestone Dec 30, 2021
@macandcheese
Copy link
Contributor

Do we need to allow this to be configured at all?

Should users just be adding and removing the element, or does having the ability to toggle visibility via prop aid in a transition or fade-in or something? cc @asangma

@driskull
Copy link
Member

Do we need to allow this to be configured at all?

I don't think so. We just choose what we want to do.

Should users just be adding and removing the element, or does having the ability to toggle visibility via prop aid in a transition or fade-in or something?

That's a good question. If a user had to construct a calcite-modal at the time that the user wanted to display it would that be problematic? It might be an inconvenience. Currently, a user can just write the HTML for a modal and then enable it at a later time.

As for hidden vs active... hidden will set display:none which might be problematic for animations of modals/alerts/tooltips/popovers. I think that's why we are using our own property, to handle animation. We might be able to do animation using hidden but it might be a little tricky.

@driskull
Copy link
Member

In the case of calcite-loader, we probably don't need an active property and can just use the hidden attribute.

I don't see any animation going on when setting active for this component. It's also not a component that contains any content, so there's no reason not to just remove it or set hidden property on it.

@jcfranco jcfranco changed the title [calcite-loader] Drop active in favor of hidden [calcite-loader, calcite-input-message] Drop active in favor of hidden Jul 23, 2022
@jcfranco
Copy link
Member Author

I updated the issue to include calcite-input-message. I think the other components brought up earlier can stay as is since, active/open is linked to their behavior . Loader and input message are more like atoms that can be displayed/hidden by an owning component.

@jcfranco jcfranco removed the design Issues that need design consultation prior to development. label Jul 26, 2022
@jcfranco jcfranco assigned Elijbet and unassigned macandcheese Jul 26, 2022
@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Jul 26, 2022
@jcfranco jcfranco removed the discussion Issues relating to a conversation where feedback is optional. label Jul 26, 2022
@github-actions github-actions bot removed this from the Sprint 2022/08/08 - 2022/08/19 milestone Aug 22, 2022
@Elijbet Elijbet added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Oct 25, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@github-actions github-actions bot assigned geospatialem and unassigned Elijbet Oct 25, 2022
@geospatialem
Copy link
Member

This update looks great, nice work, @Elijbet! ✨

@jcfranco Wondering since the active prop is deprecated, and will likely no longer be supported in the future if this would constitute a breaking change, since users will have to toggle hidden as opposed to active?

@geospatialem
Copy link
Member

Re-assigning to revert prior to the upcoming release for additional testing and follow-up.

@geospatialem geospatialem assigned Elijbet and unassigned geospatialem and benelan Oct 25, 2022
@geospatialem geospatialem added 2 - in development Issues that are actively being worked on. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Oct 25, 2022
@Elijbet Elijbet added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Nov 17, 2022
@github-actions github-actions bot assigned benelan and geospatialem and unassigned Elijbet Nov 17, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Nov 17, 2022
@geospatialem
Copy link
Member

Verified on next.634 for loader and input-message! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

No branches or pull requests

6 participants