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

SelectPanel: Empty State - Explore SelectPanel.Message API and managing visibility of the component under the hood #4988

Closed
wants to merge 87 commits into from

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Sep 19, 2024

Closes #

Changelog

New

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented Sep 19, 2024

⚠️ No Changeset found

Latest commit: 9895c58

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the staff Author is a staff member label Sep 19, 2024
@github-actions github-actions bot temporarily deployed to storybook-preview-4988 September 19, 2024 04:17 Inactive
@broccolinisoup broccolinisoup changed the base branch from main to select_panel_loading_states September 19, 2024 04:26
Comment on lines +454 to +459
<SelectPanel.Message variant="noitems" title="You haven't created any projects yet">
<Link href="https://github.com/projects">Start your first project </Link> to organise your issues.
</SelectPanel.Message>
<SelectPanel.Message variant="nomatches" title={`No language found for `}>
Adjust your search term to find other languages
</SelectPanel.Message>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spiking about API options for the empty states. After having an initial conversation with @siddharthkp

Option 1 (As above code)

Pros

  • Straightforward. Consumers don't need to deal with calculating items length etc to display the empty state messages and which empty state they are going to display i.e. no items vs no matches

Cons

  • No flexibility to manage the visibility of the messages since everything is under the hood. There is a chance that it might not work for every usage if consumers have more params in determining the empty states. They can still manage it via this API but it would defeat its main motivation which is handling the complex stuff under the hood.

Option 2

We could simplify the API and let consumers worry about managing the empty state. Like below

<SelectPanel.Message variant="empty" title={noItemsState? `No items created yet' : No language found for'`}>
      {noItemsState? `Start creating your items here'  : Adjust your search term to find other languages}
</SelectPanel.Message>

Pros

  • Simple API

Cons

  • Consumers might choose to use the same messages for both empty states.
  • A bit more work for consumers to deal with managing the state.

Alternatively we can use a prop rather than children for providing the messages.

<SelectPanel
  message={<SelectPanel.Message variant="empty" title={noItemsState? `No items created yet' : No language found for'`}>
      {noItemsState? `Start creating your items here'  : Adjust your search term to find other languages}
</SelectPanel.Message>}
</SelectPanel>

Curious to hear what you think 🙌🏻 @siddharthkp @camertron

Copy link
Member

@siddharthkp siddharthkp Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like option 2 a lot more, it's more intuitive for me to assume what I ever I give in JSX will be rendered.

A bit more work for consumers to deal with managing the state.

For me, the way to remove extra work would be be more configuration based and asked for messages object

<SelectPanel
  message={{
     noInitialItems: {title: 'No items created yet', body: <><Button>Start creating items</Button></>},
     noFilteredItems: {title: `No items found for ${query}`, body: <>Adjust your search term to find other languages</>}
  }}
>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the classic question that always comes up when designing component APIs: how much freedom should we give consumers? In my experience, the more freedom we give consumers, the greater the chance for incongruous outcomes.

This case is even trickier than most because we can almost get away with a props-only approach. Unfortunately the designs show that consumers should be able to add links to the message description, and for error messages it would be nice to eventually be able to add a "Try again" button, etc etc. So I think we have to allow arbitrary content for both error messages and no items/matches.

That being said, IMO we can still establish some guardrails. The designs have two bits of text, a title and a description, with specific font colors and sizes. Whatever API we choose, let's make it easy to do the default thing, eg. provide plaintext values for these fields, and style them appropriately.

As far as behavior goes, I'm firmly in the camp of having the component manage showing/hiding the messages based on the current filter. Eg. if there is no filter text, show the "no items" message; if there is filter text, show the "no matches" message. I would really prefer to reduce the amount of knowledge a consumer has to have about how select panels are supposed to work in order to use them correctly. Requiring consumers to make decisions about when messages are shown or hidden exacerbates this knowledge gap and will lead to problems. Not only will they have to understand the API, but also implicit rules about how to use it appropriately that they may or may not have read in the documentation. The API should be clear enough to (largely) speak for itself.

@broccolinisoup and I brainstormed this during our 1:1 on Monday and came up with an approach where each message has two slots, a title and a description. I imagine it would look something like this:

<SelectPanel.Message variant="no-items">
  <SelectPanel.Message.Title>
    No projects found.
  </SelectPanel.Message.Title>
  <SelectPanel.Message.Description>
    Would you like to <Link href="/projects/create">create one</Link>?
  </SelectPanel.Message.Description>
</SelectPanel.Message>

That's kind of verbose. Maybe we can simplify it?

Hopefully that resonates with you @siddharthkp! Always willing to have my mind changed tho 😄

I like option 2 a lot more, it's more intuitive for me to assume what I ever I give in JSX will be rendered.

Yeah, I get that. Hopefully an approach like the one I outlined above will still give consumers the ability to pass in custom JSX and have it be rendered as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you thank you both!! I gathered everything we chatted and wrote down as a discussion #5031 so that we can have a little bit nicer place to continue our discussion and make a decision.

<SelectPanel.Message variant="noitems" title="You haven't created any projects yet">
<Link href="https://github.com/projects">Start your first project </Link> to organise your issues.
</SelectPanel.Message>
<SelectPanel.Message variant="nomatches" title={`No language found for `}>
Copy link
Member

@siddharthkp siddharthkp Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, interesting.

  1. Does this mean we would figure out which Message to render and ignore the rest?
  2. Am I correct in assuming we would append the query string at the end of this title?title={'No language found for '}>

Copy link
Member Author

@broccolinisoup broccolinisoup Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Actually it is similar to what you have mentioned in your comment - to have the data for both state. Here is more of a variant version and yours is coming from a message object if I understand it right. Tbh, I don't like this API very much. It is cumbersome. I am just entertaining various options 😄

Consumer don't have to figure out which message to render, we will have defaults but if they need to customise this is how they can do (in this API option)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yeah!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this seems great! My only suggestion is to add individual slots (children) for the title and description.

Base automatically changed from select_panel_loading_states to main October 10, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staff Author is a staff member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants