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

Consolidate closing/removing approach across components #6789

Open
jcfranco opened this issue Apr 14, 2023 · 5 comments
Open

Consolidate closing/removing approach across components #6789

jcfranco opened this issue Apr 14, 2023 · 5 comments
Labels
0 - new New issues that need assignment. blocked This issue is blocked by another issue. estimate - 8 Requires input from team, consider smaller steps. future breaking change Issues and pull requests with deprecation(s), requires a breaking change in a future milestone. refactor Issues tied to code that needs to be significantly reworked.
Milestone

Comments

@jcfranco
Copy link
Member

jcfranco commented Apr 14, 2023

Description

This stems from #6775 (comment):

We should consolidate how we handle closed/removed items. I've noticed we have the following discrepancies:

  1. naming: closable vs removable
  2. approach: hiding vs removing the node

cc @driskull

Proposed Advantages

Consistency.

Which Component

All closables/removables:

component prop name approach
alert closable hidden
chip closable hidden
flow-item removable removed
list-item closable hidden
modal (deprecated) closable hidden
notice closable hidden
panel closable removed
pick-list-item (deprecated) closable removed
popover closable hidden
tip (deprecated) closedDisabled hidden
value-list-item (deprecated) removable removed

Relevant Info

No response

@jcfranco jcfranco added 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 Apr 14, 2023
@driskull
Copy link
Member

I think the only "removable" ones are flow-item and value-list-item. We can possibly ignore value-list-item as we plan on deprecating it. flow-item is pretty unique and we did have an issue to change it to not remove any child elements but we kind of scrapped that for now. We could revisit it but it would be a breaking change.

@driskull
Copy link
Member

A lot of the nodes where we set them to hidden instead of removing them is because inside of those nodes we have a slot element and we don't want to remove the slot element conditionally in order to keep using onslotchange events.

@geospatialem geospatialem added estimate - 8 Requires input from team, consider smaller steps. future breaking change Issues and pull requests with deprecation(s), requires a breaking change in a future milestone. labels Apr 2, 2024
@geospatialem geospatialem added blocked This issue is blocked by another issue. and removed needs triage Planning workflow - pending design/dev review. labels Apr 30, 2024
@geospatialem geospatialem added this to the Stalled milestone Apr 30, 2024
@geospatialem
Copy link
Member

Blocked by the efforts of #6473.

@eriklharper
Copy link
Contributor

eriklharper commented Oct 29, 2024

I'm just now learning about certain parent <> child component relationships where slotted children are physically removed from the DOM, and my first instinct is that this is an anti-pattern because the parent doesn't own the slotted children, the HTML author does, so shouldn't it be up to the HTML author to perform the node removal rather than the calcite component? I'm thinking of VDOM use cases too, where there is an expectation that the DOM reflects the app's internal state.

I imagine the flow of operations to be:

  1. End user initiates a "close" action on an item
  2. Child component triggers a "close" event
  3. Parent listens for "close" event on child, possibly fires an additional event and swallows child event
  4. HTML author's code responds to the close event, updating internal state where necessary
  5. HTML author's rendering logic updates the DOM to reflect the new state, removing the closed node.

@driskull
Copy link
Member

Yes, its an anti pattern. flow-item is the only one doing this and it will be fixed in 3x with a breaking change. At 3x, we can close this issue. #9390 #5072

panel doesn't remove elements so this chart should be updated.

I think we can close this issue and rely on #5072 cc @geospatialem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - new New issues that need assignment. blocked This issue is blocked by another issue. estimate - 8 Requires input from team, consider smaller steps. future breaking change Issues and pull requests with deprecation(s), requires a breaking change in a future milestone. refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

No branches or pull requests

4 participants