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

feat(value-list-item): add new slot labelled as content-center to replace label/description (#3373) #3579

Closed
wants to merge 32 commits into from

Conversation

y0n4
Copy link
Contributor

@y0n4 y0n4 commented Nov 29, 2021

Related Issue: #3373

Summary

In the case of when the label and description prop is not utilized, there is a custom content slot that can replace the area of the label/description. This way there won't be a big gap in the middle of the component and no need to squish more content to the other slots like actions-start or actions-end. Label/description or content slot must be provided while using the calcite-pick-list-item component

Content slot will take precedence over label/description if provided.
When the list item is interactive and has content slot, the item can be selected if user clicks outside of the content slot.

Before: you can see that there is an empty gap where the label/description is supposed to be and the user wants to include more content
Screen Shot 2021-12-13 at 11 09 49 AM 2
After: now label/description can be replaced with custom content and there is no longer that empty gap
Screen Shot 2021-12-03 at 1 52 06 PM 2

@y0n4 y0n4 requested a review from a team as a code owner November 29, 2021 22:23
@github-actions github-actions bot added this to the Sprint 11/22 - 12/3 milestone Nov 29, 2021
@driskull
Copy link
Member

@y0n4 can you add a test for this? Maybe a screener update to show an item without a title?

@jcfranco
Copy link
Member

@asangma Can you confirm if having no content still fits with the standardized item layout pattern

[action-start][content-start][content][content-end][action-end])

?

@asangma
Copy link
Contributor

asangma commented Dec 1, 2021

If I understand correctly, you want to know if no label or description is added, the actions-start and actions-end slots should fill the available space in, yes?

As far as that layout pattern, that' pretty much it, though the full pattern wraps the content slots and node in a button.
[actions-start] [ button [content-start][content][content-end] ] [actions-end]

@y0n4
Copy link
Contributor Author

y0n4 commented Dec 2, 2021

@asangma yes that is correct. Should the component require a label or can it be empty and the slots will stretch the rest of the space? There is that whole empty gap around the label area because it is expecting a label. The e2e test is also expecting a label as well so if it's not required, the test will fail

It is a bit confusing since label prop is required in calcite-value-list-item (which is utilized in this issue use case) but not required in calcite-pick-list-item which is basically the entire structure of calcite-value-list-item component. The consistency is a bit off

@asangma
Copy link
Contributor

asangma commented Dec 2, 2021

Seems like we should just require it in PickListItem.

@asangma
Copy link
Contributor

asangma commented Dec 2, 2021

Because the request is focused on allowing custom content in the main content area, I suggest that we

  • do not require label
  • we render the default slot in the content area

This way a user can do like

<calcite-pick-list-item>
  <calcite-action slot="actions-end"></calcite-action>
  <calcite-label>
    <calcite-input />
  </calcite-label>
  <!-- or whatever -->
</calcite-pick-list-item>

@y0n4 y0n4 changed the title style: minimize title/description space on the list when prop value is not provided (#3373) enhancement(value-list-item): add new slot labelled as content-center to replace label/description (#3373) Dec 6, 2021
@y0n4 y0n4 marked this pull request as draft December 7, 2021 00:16
@y0n4 y0n4 changed the title enhancement(value-list-item): add new slot labelled as content-center to replace label/description (#3373) feat(value-list-item): add new slot labelled as content-center to replace label/description (#3373) Dec 7, 2021
@y0n4 y0n4 marked this pull request as ready for review December 7, 2021 20:15
@y0n4 y0n4 marked this pull request as draft December 7, 2021 22:16
@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

@driskull I'm thinking that if the content slot is used, we just render whatever they put in there and don't do anything with label props or click handlers. Basically, if you wanna take control of that, you have control over behavior.

@driskull
Copy link
Member

@asangma I think some more research needs to happen here. The role of a pick-list-item is a checkbox.

If a user slots an input inside of a pick-list-item, then its basically like putting a input inside a checkbox.

If we remove the checkbox role, then it's no longer able to "pick".

The purpose of this slot needs to be clarified and limited otherwise we are setting users up for a11y failures and breaking changes down the road for us.

@driskull
Copy link
Member

If a input needs to be slotted, it needs to go next to the checkbox role, not inside it.

@driskull
Copy link
Member

@benelan @jcfranco we might need to require more usage of the accessible test helper. So that when a new slot is added its a11y is tested. Probably a convention update here.

@jcfranco
Copy link
Member

Apologies for not chiming in sooner. I'll review first thing tomorrow.

@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

@jcfranco @driskull I think we need to revisit the design and implementation of this. Chatted with @y0n4.

@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

Here is the basic concept. I'd like to run the UI (see below) through a design review.
image

I have some initial UI stuff here.

@driskull
Copy link
Member

Maybe two separate item types since the UI will be different depending on a slot being used? I'm not sure where we draw the line and say its a separate item component rather than it renders differently when things are slotted.

@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

I considered that. However, one of the workflows/UX in question is being able to do some kind of inline editing, e.g. temporarily replace the standard content with an input.

@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

I'd be willing to remove the select/deselect button when "content" slot is used.
That'd be the most straightforward approach I think.

@macandcheese
Copy link
Contributor

At what point is a regular list item more apt?

@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

@macandcheese A dev could do that, but ListItem doesn't come with all the single-select, multi-select and prescribed icons (check vs radio).

@asangma
Copy link
Contributor

asangma commented Dec 30, 2021

Really, this is a pattern we use in a couple other places, i.e. having a slot that overrides a main content area.

That being said, I'm open to other solutions for the initial request to have inline editable text.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Jan 7, 2022
@jcfranco
Copy link
Member

Blocking and freezing this one until we flesh out the details for the action/content slot pattern epic. @y0n4 @asangma @driskull @macandcheese Thanks for the time and energy on this, but the implementation of this brought up some items that needed to be well-defined first.

@jcfranco jcfranco added the blocked This issue is blocked by another issue. label Jan 27, 2022
@github-actions github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Jan 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2022

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Feb 5, 2022
@y0n4 y0n4 closed this Mar 4, 2022
@y0n4 y0n4 removed their assignment Mar 4, 2022
@driskull driskull deleted the y0n4/3373-value-list-label branch May 18, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue is blocked by another issue. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants