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

docs(ListView): API Decision #2453

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

docs(ListView): API Decision #2453

wants to merge 15 commits into from

Conversation

saurabhdaware
Copy link
Member

@saurabhdaware saurabhdaware commented Dec 26, 2024

Formatted Documents

Rough Work

<FilterChipDatePicker />

<Dropdown>
  <FilterChipSelectInput />
  <DropdownOverlay>
  </DropdownOverlay>
</Dropdown>

// release later
<Menu>
  <FilterChip />
</Menu>

TODO:

  • Check with RK if there are checkboxes in FilterChip

Copy link

changeset-bot bot commented Dec 26, 2024

⚠️ No Changeset found

Latest commit: ce2c86a

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

Copy link
Contributor

github-actions bot commented Dec 26, 2024

✅ PR title follows Conventional Commits specification.

Copy link

codesandbox-ci bot commented Dec 26, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ce2c86a:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Dec 26, 2024

Bundle Size Report

No bundle size changes detected.

Generated by 🚫 dangerJS against ce2c86a

- Less verbose compared to API without layout component

**Cons:**

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really a con? Lets check with design if this will ever be needed to move things around within quick filters

## Enhancements / Components

- Enhancements:
- Searchable Dropdown
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isnt that just autocomplete? What do we need to do here?


- Enhancements:
- Searchable Dropdown
- InputGroup (TBD)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this needed?

- Enhancements:
- Searchable Dropdown
- InputGroup (TBD)
- New Components
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering the number of new components and enhancements here, our estimate of 2 weeks seems less. Lets re-evaluate the estimates?

</Dropdown>
```

### FilterChipGroup
Copy link
Collaborator

Choose a reason for hiding this comment

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

There will be different types of filter chips, right? Will this API suffice for all the types?

</Dropdown>
```

### FilterChipGroup
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is the naming convention on Figma for this? Are we also call this FilterChipGroup & FilterChip?

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: make names consistent on figma and dev

/* filter on search */
}}
>
<FilterGroup />
Copy link
Member

Choose a reason for hiding this comment

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

What is FilterGroup?

Comment on lines +396 to +398
onSearch={({ value, searchType }) => {
/* filter on search */
}}
Copy link
Member

@anuraghazra anuraghazra Jan 13, 2025

Choose a reason for hiding this comment

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

image

@saurabhdaware I believe we should keep dev side a bit more flexible with slots.

For example in many places in X we don't need search but rather we have some action buttons.

We can keep the QuickFilters and SearchGroup into separate slots which can be independently be used.

<ListView> // internally it will have justify-content: space-between
  <ListViewQuickFilters
    filters={[
      {
        title: 'All',
        onClick: () => {},
      },
      {
        title: 'Pending',
        onClick: () => {},
      },
    ]}
  />
  <Box>
    <ListViewFilterToggle />
    <ListViewSearchGroup />
  </Box>
</ListView>

not exactly but something like this ^ this way I can use QuickFilters independently of ListView.

Or

<ListView> // internally it will have justify-content: space-between
  <ListViewFilters>
    <QuickFilters
      filters={[
        {
          title: 'All',
          onClick: () => {},
        },
        {
          title: 'Pending',
          onClick: () => {},
        },
      ]}
    />
    <ListViewFiltersActions> // control the gap also
      <ListViewFilterToggle />
      <ListViewSearchGroup />
    </ListViewFiltersActions>
  </ListViewFilters>
</ListView>

```ts
type QuickFilter = {
title: string;
onClick: (e: React.MouseEvent) => void;
Copy link
Member

Choose a reason for hiding this comment

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

We will also need onChange, value & controlled/uncontrolled state for QuickFilters.

Because QuickFilters will have the same semantics of RadioGroup it can also change on keyboard events not just click.

Plus this will also be a requirement if we want this to work independently as i've mentioned here

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. Will change

onClick: () => {},
},
]}
onSearchChange={() => {
Copy link
Member

Choose a reason for hiding this comment

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

We need to make sure consumers are able to externally control all aspects of quickfilters/search etc.

Quick filters, search, and dropdowns may be controlled or have initial values set via URL query parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO:

  • Check search feasibility (design @-RK and API)
  • Check what all might come in trailing of QuickFilters

Comment on lines +280 to +298
<FilterChipGroup>
<Dropdown>
<FilterChip value={duration} onClearButtonClick={({ value }) => setDuration(undefined)}>
Duration
</FilterChip>
<DropdownOverlay>
<ActionList>
<ActionListItem
title="2 days"
value="2d"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 week"
value="1w"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 month"
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn’t this get quite cumbersome to write all these JSX even when we just have 2-3 filter chips, not to mention most production apps will have 8-12 chips

Are there any alternatives we can approach? 🤔 Since we already know the specific inputs involved, could we consider a more streamlined, config-driven, and focused API instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will check with RK on where we need freeflowing texts + leading / trailing usage on actionlist

Copy link
Member Author

@saurabhdaware saurabhdaware left a comment

Choose a reason for hiding this comment

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

Ignore the irrelevant comments. Old comments. I forgot to submit these.

onClick: () => {},
},
]}
onSearchChange={() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO:

  • Check search feasibility (design @-RK and API)
  • Check what all might come in trailing of QuickFilters

```ts
type QuickFilter = {
title: string;
onClick: (e: React.MouseEvent) => void;
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. Will change

Comment on lines +280 to +298
<FilterChipGroup>
<Dropdown>
<FilterChip value={duration} onClearButtonClick={({ value }) => setDuration(undefined)}>
Duration
</FilterChip>
<DropdownOverlay>
<ActionList>
<ActionListItem
title="2 days"
value="2d"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 week"
value="1w"
onClick={({ name, value }) => setDuration(name)}
/>
<ActionListItem
title="1 month"
Copy link
Member Author

Choose a reason for hiding this comment

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

Will check with RK on where we need freeflowing texts + leading / trailing usage on actionlist

</Dropdown>
```

### FilterChipGroup
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: make names consistent on figma and dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants