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

Audit components open/closed props for consistency #6473

Open
SkyeSeitz opened this issue Feb 14, 2023 · 29 comments
Open

Audit components open/closed props for consistency #6473

SkyeSeitz opened this issue Feb 14, 2023 · 29 comments
Labels
0 - new New issues that need assignment. Calcite (design) Issues logged by Calcite designers. design Issues that need design consultation prior to development. enhancement Issues tied to a new feature or request. estimate - 5 A few days of work, definitely requires updates to tests. low risk Issues with low risk for consideration in low risk milestones p - medium Issue is non core or affecting less that 60% of people using the library ready for dev Issues ready for development implementation. spike complete Issues that have a research spike completed and dev work can proceed

Comments

@SkyeSeitz
Copy link

SkyeSeitz commented Feb 14, 2023

Description

An audit to define open and open/closed verses expanded/collapsed properties and use consistently across Calcite components.

Audit summary: The audit effort was completed in June 2024, and deprecations and new functionality will be added during the 3.0 breaking change release, with anticipated removal in the 4.0 breaking change release.

Blocked issues: #6473, #4531, #6789

cc @geospatialem @ashetland @macandcheese @jcfranco

Acceptance Criteria

New components and props should follow this updated pattern:
open/close: Toggles visibility of entire component, or its menu
expand/collapse: Toggles visibility of more content within a component


Certain components may have need for both an open/close state as well as a expanded/collapsed such as:
[list-item] can be open (component is visible) & collapsed (child items are hidden) at the same time
[panel] can be open (component is visible) & collapsed (content area is hidden) at the same time

Action items for 3.0:
[list-item] deprecate open & add functionality to expanded
[block] deprecate open & add functionality to expanded
Consider an eslint rule to ensure a component does not have both open/closed -or- expanded/collapsed

Events:
Leverage the open/close utility for expanded and collapsed. Likely just needs some small functional tweaks. Refer to Eliza's comment below for more context.

Link to Figma file
image

Which Component

Applies across multiple components:
list-item, block

Esri team

Calcite (design)

@SkyeSeitz SkyeSeitz added enhancement Issues tied to a new feature or request. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Feb 14, 2023
@github-actions github-actions bot added the Calcite (design) Issues logged by Calcite designers. label Feb 14, 2023
@SkyeSeitz SkyeSeitz added p - low Issue is non core or affecting less that 10% of people using the library and removed Calcite (design) Issues logged by Calcite designers. labels Feb 14, 2023
@github-actions github-actions bot added the Calcite (design) Issues logged by Calcite designers. label Feb 14, 2023
@SkyeSeitz
Copy link
Author

SkyeSeitz commented Feb 14, 2023

This is a proposal to start the discussion with follow up questions:
image

  1. Depending on how we define open, does anything in the open category fit better in active or expanded?
  2. In the case of Action's active prop, how to we define the difference between its role and a selected prop?

@SkyeSeitz SkyeSeitz changed the title Audit components open/`closed props for consistency Audit components open/closed props for consistency Feb 14, 2023
@geospatialem geospatialem added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Mar 30, 2023
@anveshmekala
Copy link
Contributor

During our API consistency audit, we defined expanded for components which expand or collapse within the same area as component where as open is used for component which display a popup or content outside of component with a trigger button. These are the initial thoughts and open might just work for all the use cases.

@SkyeSeitz
Copy link
Author

During our API consistency audit, we defined expanded for components which expand or collapse within the same area as component where as open is used for component which display a popup or content outside of component with a trigger button. These are the initial thoughts and open might just work for all the use cases.

Thanks, @anveshmekala! By all use cases do you mean including Block, Accordion Item, Tree, List, and Action Bar? Those sound like they fit your description of expanded, correct?

@ashetland
Copy link

I think the original logic still makes sense, should we decide to stick with it. I think we can be more consistent in implementing it though. Based on our audit, I believe the inconsistencies are on Block and List. Both use open, but I believe expanded would be correctly following the original logic.

Tip feels like an outlier here. Its props are close-disabled and closed which is at odds with closable that's used, I think, everywhere else.

@SkyeSeitz
Copy link
Author

Looking at Tip again, you are right, it should be in its own category to consider changing to closable if possible. I have updated the proposal image above.

@macandcheese
Copy link
Contributor

macandcheese commented Apr 6, 2023

Panel, Chip, Notice, etc., also have closable. I'd hold off on any updates to the Tip component as it will likely end up seeing significant change / rename : #6536

@anveshmekala
Copy link
Contributor

During our API consistency audit, we defined expanded for components which expand or collapse within the same area as component where as open is used for component which display a popup or content outside of component with a trigger button. These are the initial thoughts and open might just work for all the use cases.

Thanks, @anveshmekala! By all use cases do you mean including Block, Accordion Item, Tree, List, and Action Bar? Those sound like they fit your description of expanded, correct?

they does fit the definition of expansion, but there is a discussion of using just open to limit the confusion.

cc : @jcfranco

@anveshmekala
Copy link
Contributor

I think the original logic still makes sense, should we decide to stick with it. I think we can be more consistent in implementing it though. Based on our audit, I believe the inconsistencies are on Block and List. Both use open, but I believe expanded would be correctly following the original logic.

Tip feels like an outlier here. Its props are close-disabled and closed which is at odds with closable that's used, I think, everywhere else.

The reason behind using closed and closeDisabled for tip is to make sure the prop names makes sense for the default value of false, we have a convention at the code level to not use props whose default value is true.

if we replace closeDisabled with closable then the default value has to be true

@SkyeSeitz SkyeSeitz added the ready for dev Issues ready for development implementation. label Jun 1, 2023
@SkyeSeitz
Copy link
Author

Updated the issue description to complete this design proposal.

Copy link
Contributor

github-actions bot commented Nov 6, 2023

cc @geospatialem, @brittneytewks

@brittneytewks brittneytewks removed the needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. label Nov 6, 2023
@brittneytewks brittneytewks removed the figma changes Issues that require additions or updates to the Figma UI Kit where no `design` label exists label Dec 12, 2023
@geospatialem geospatialem added the spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. label May 20, 2024
@Elijbet
Copy link
Contributor

Elijbet commented Jun 4, 2024

I wasn't aware we had components with closed/collapsed props instead of open/expanded. I don't think the openClose commonTest util accounts for those cases. openPropName currently checks for either "open" or "expanded".
tip, chip, and flow-item that use closed don't have openClose tests set up for them. Neither do panel and shell-panel that have collapsed. flow-item has both collapsed and close.

Submitting this issue towards the effort: Expand openClose commonTest util to account for components with closed/collapsed props #9511

Also propose introducing clarity in event naming for expanded/collapsed vs open/closed: Revisit event names emitted by openCloseComponent util #9513

@driskull
Copy link
Member

driskull commented Jun 6, 2024

Nice catch @Elijbet. We need to add those events for sure

@geospatialem geospatialem added breaking change Issues and pull requests with code changes that are not backwards compatible. spike complete Issues that have a research spike completed and dev work can proceed labels Jun 20, 2024
@github-actions github-actions bot added needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. and removed spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. labels Jun 20, 2024
@github-actions github-actions bot removed this from the 2024-06-25 - Jun Release milestone Jun 20, 2024
Copy link
Contributor

cc @geospatialem, @brittneytewks

@geospatialem geospatialem added this to the 2024-11-19 - Nov Release milestone Jun 20, 2024
@geospatialem geospatialem added estimate - 5 A few days of work, definitely requires updates to tests. p - medium Issue is non core or affecting less that 60% of people using the library and removed breaking change Issues and pull requests with code changes that are not backwards compatible. needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. p - low Issue is non core or affecting less that 10% of people using the library labels Jun 20, 2024
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. Calcite (design) Issues logged by Calcite designers. design Issues that need design consultation prior to development. enhancement Issues tied to a new feature or request. estimate - 5 A few days of work, definitely requires updates to tests. low risk Issues with low risk for consideration in low risk milestones p - medium Issue is non core or affecting less that 60% of people using the library ready for dev Issues ready for development implementation. spike complete Issues that have a research spike completed and dev work can proceed
Projects
None yet
Development

No branches or pull requests

10 participants