Open SkyeSeitz opened 1 year ago
This is a proposal to start the discussion with follow up questions:
open
, does anything in the open
category fit better in active
or expanded
?active
prop, how to we define the difference between its role and a selected
prop?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.
During our API consistency audit, we defined
expanded
for components which expand or collapse within the same area as component where asopen
is used for component which display a popup or content outside of component with a trigger button. These are the initial thoughts andopen
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?
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.
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.
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 : https://github.com/Esri/calcite-components/issues/6536
During our API consistency audit, we defined
expanded
for components which expand or collapse within the same area as component where asopen
is used for component which display a popup or content outside of component with a trigger button. These are the initial thoughts andopen
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
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 believeexpanded
would be correctly following the original logic.Tip feels like an outlier here. Its props are
close-disabled
andclosed
which is at odds withclosable
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
Updated the issue description to complete this design proposal.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
This issue has been automatically closed due to inactivity.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Chiming in to confirm this issue is still relevant and probably still p-low.
@SkyeSeitz WRT the updated description:
open (Reveals a floating element) Applies to: Combobox, Input Date Picker, Input Time Picker, Modal, Dropdown, Popover, & Tooltip
expanded (Reveals extended content) Applies to: Accordion, Block, Tree Item, List Item, & Action Bar
As @anveshmekala mentioned earlier, I was hoping we could use open
for both, so what is the driver for making the distinction between these properties?
open
/expanded
on a component?Good point @jcfranco! The goal was to achieve consistency and I must have totally missed that we were moving away from the need to distinguish between things the original definitions provided - I am on board to simplify to "open" across all listed components since..
open/expand
that is relevant to our consumers open/expanded
on a component at the same timeThanks for chiming in, @SkyeSeitz!
Based on the above, I think we should pursue using open
consistently across components.
Awesome. I have updated the description above 🙌🏽
@driskull brought up some valid use cases against point 2:
open
, which controls whether it's displayed or not.open
and collapsible
, which both affect the visual collapsed state.open
(toggles collapsible state) and closed
(toggles visibility) 🥲We'd definitely need a way to distinguish these for future cases. We could establish the following guidelines:
open
/closed
➡️ toggles visibility of the component or a part of the component (e.g., menu).expanded
/collapsed
➡️ toggles a section of the component that is visually expanded/collapsedThe prop name is determined by the default false value (since we avoid true as a default for booleans).
If a component falls under 1 and has a subcomponent that also qualifies under 1, the parent component should use <part><Open|Closed|Expanded|Collapsed>
.
We can iterate on this and once we feel it's solid we can start applying to components for new props. Depending on the audit, might be able to fix some of these by deprecating in favor of the preferred prop. For the cases where we can't deprecate, we would tackle at a later breaking change release.
cc @Elijbet
Solid logic methinks 🏴☠️
Very good points, Matt & Franco! Thanks for brining those to light 💡
open
/closed
➡️ toggles visibility of the component or a part of the component (e.g., menu).expanded
/collapsed
➡️ toggles a section of the component that is visually expanded/collapsed
The use-case of having both open
and expanded
in a component will benefit from defining the usage of open
/closed
vs expanded
/collapsed
. I don't think currently any component support both of them or atleast during 1.0. By supporting both are we scraping the idea of having only open
for all use-cases?
By supporting both are we scraping the idea of having only open for all use-cases?
Yes. Once #7498 lands, we'll have a component with props in the open/close and expand/collapse groups each behaving as described above. We won't be able to do this with a single prop.
Along those lines, we also need to prioritize guidelines/conventions behind open
/close
, expand
/collapse
, its associated events to apply to components moving forward and also have a separate list/issue to track inconsistencies and how we'd like to fix at a later point. cc @Elijbet
Just learned that Action Menu already has both expanded
and open
props.
Updated the description to reflect discussion above. @jcfranco what would you recommend for next steps? Label as needs triage so a dev and designer can be assigned to develop the guidelines/conventions behind open/close
, expand/collapse
together?
cc @geospatialem, @brittneytewks
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
Nice catch @Elijbet. We need to add those events for sure
cc @geospatialem, @brittneytewks
Description
An audit to define open and
open
/closed
versesexpanded
/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 the4.0
breaking change release.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 menuexpand
/collapse
: Toggles visibility of more content within a componentCertain components may have need for both an open/close state as well as a expanded/collapsed such as:
[list-item]
can beopen
(component is visible) &collapsed
(child items are hidden) at the same time[panel]
can beopen
(component is visible) &collapsed
(content area is hidden) at the same timeAction items for
3.0
:[list-item]
deprecateopen
& add functionality toexpanded
[block]
deprecateopen
& add functionality toexpanded
Consider an eslint rule to ensure a component does not have bothopen
/closed
-or-expanded
/collapsed
Events: Leverage the
open
/close
utility forexpanded
andcollapsed
. Likely just needs some small functional tweaks. Refer to Eliza's comment below for more context.Link to Figma file![image](https://github.com/Esri/calcite-design-system/assets/29716057/f68ae8f5-b6ef-49c0-a5be-1f60f814c1ef)
Which Component
Applies across multiple components: list-item, block
Esri team
Calcite (design)