Skip to content

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented Oct 3, 2023

Related to https://github.com/Shopify/polaris-internal/issues/999

Before
image

This PR
index-filters-loading-demo

Filters mode index-filters-loading-1
Search mode, no input index-filters-loading-2
Search mode, with input index-filters-loading-3

@mrcthms
Copy link
Contributor

mrcthms commented Oct 3, 2023

@jesstelford I can offer some context around this – we didn't want to have the input flicker lengths when the table is in a loading state, but rather have a consistent length. I'll double check with @ouellettejordan whether this is something we want to keep, as I know this was an intentional ask when we initially built the component.

{loading ? <Spinner size="small" /> : null}
</div>
{loading ? (
<div className={styles.Spinner}>
Copy link
Member

@chloerice chloerice Oct 3, 2023

Choose a reason for hiding this comment

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

What if the spinner replaced the input clear button when loading is true, or rendered to the left of the clear button?

replacing left of
demo-replacing-clear-button-on-load.mp4
demo-spinner-left-of-clear-button.mp4

@jesstelford jesstelford force-pushed the indexfilters-spinner-space branch from 23d1fbd to 1b46f82 Compare October 4, 2023 00:59
@jesstelford jesstelford changed the title [IndexFilters] Loading spinner no longer takes up horizontal space when !loading. [IndexFilters] Loading spinner moved to be a suffix within the search box. Oct 4, 2023
@jesstelford
Copy link
Contributor Author

Great input @mrcthms & @chloerice!

I've moved the spinner to be next to the close button, what do you think?

index-filters-loading-demo

Filters mode index-filters-loading-1
Search mode, no input index-filters-loading-2
Search mode, with input index-filters-loading-3

@jesstelford
Copy link
Contributor Author

jesstelford commented Oct 4, 2023

🤔 The CI failures don't make sense me - I don't get that locally, and "2" should be a valid gap type... Any ideas? Rebase onto next fixed it 👍

@jesstelford jesstelford force-pushed the indexfilters-spinner-space branch 3 times, most recently from 50e8c13 to f7e2447 Compare October 4, 2023 07:17
@jesstelford jesstelford force-pushed the indexfilters-spinner-space branch from f7e2447 to c01b107 Compare October 4, 2023 07:32
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this Jess!! 🙌🏽

placeholder?: string;
disabled?: boolean;
borderlessQueryField?: boolean;
/** Show a loading spinner to the right of the input */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** Show a loading spinner to the right of the input */
/** Show a loading spinner to the right of the input value */

'@shopify/polaris': patch
---

[IndexFilters] Loading spinner moved to be a suffix within the search box.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[IndexFilters] Loading spinner moved to be a suffix within the search box.
Moved the `Filters` loading spinner to the suffix of the query filter

}

.ClearButton {
.Suffix {
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Comment on lines +80 to +104
{loading || value !== '' ? (
<div className={styles.Suffix}>
<InlineStack gap="200">
{loading ? (
<div className={styles.Spinner}>
<Spinner size="small" />
</div>
) : null}
{value !== '' ? (
<UnstyledButton
className={classNames(
styles.ClearButton,
focused && styles['ClearButton-focused'],
)}
onClick={() => handleClear()}
disabled={disabled}
>
<Text as="span" visuallyHidden>
{i18n.translate('Polaris.Common.clear')}
</Text>
<Icon source={CircleCancelMinor} tone="subdued" />
</UnstyledButton>
) : null}
</InlineStack>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the markup for each of the suffix items into variables so there aren't conditionals in the return.

const loadingMarkup = loading
  ? (
    <div className={styles.Spinner}>
      <Spinner size="small" />
    </div>
  ) : null;

const clearButtonMarkup = value !== ''
  ? (
    <UnstyledButton
      onClick={() => handleClear()}
      disabled={disabled}
      className={classNames(
        styles.ClearButton,
        focused && styles['ClearButton-focused'],
      )}
    >
      <Text as="span" visuallyHidden>
        {i18n.translate('Polaris.Common.clear')}
      </Text>
      <Icon source={CircleCancelMinor} tone="subdued" />
    </UnstyledButton>
) : null

const suffixMarkup = loading || value !== '' 
  ? (
    <div className={styles.Suffix}>
      <InlineStack gap="200">
        {loadingMarkup}
        {clearButtonMarkup}
      </InlineStack>
    </div>
  ) : null;
  
  
  ....
  ....
  

{suffixMarkup}

Base automatically changed from next to main October 9, 2023 03:02
@annaShop
Copy link

hi @chloerice , could you please update on when this PR will be merged?
we have a high priority bug https://github.com/Shopify/temp-project-mover-syfiawoo-20250331132715/issues/334 that should be fixed by this merge.
thnx!

@chloerice
Copy link
Member

hi @chloerice , could you please update on when this PR will be merged? we have a high priority bug Shopify/temp-project-mover-syfiawoo-20250331132715#334 that should be fixed by this merge. thnx!

Hey @annaShop 👋🏽 The team was away Bursting last week. This will get merged in the next couple days but won't be updated in Shopify/web until the v12 upgrade ships (which will likely be early next week).

@chloerice chloerice merged commit b0d5750 into main Oct 26, 2023
@chloerice chloerice deleted the indexfilters-spinner-space branch October 26, 2023 03:09
laurkim pushed a commit that referenced this pull request Oct 30, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris-migrator@0.26.2

### Patch Changes

- [#11029](#11029)
[`ca67934e0`](ca67934)
Thanks [@lgriffee](https://github.com/lgriffee)! - Update
`v12-styles-replace-custom-property-color` migration with additional
tokens

- Updated dependencies
\[[`73b1d5d2c`](73b1d5d)]:
    -   @shopify/polaris-tokens@8.0.2
    -   @shopify/stylelint-polaris@15.0.2

## @shopify/polaris@12.0.2

### Patch Changes

- [#11037](#11037)
[`f81abddba`](f81abdd)
Thanks [@chloerice](https://github.com/chloerice)! - Added tests for
`buttonFrom` and `buttonsFrom` utility functions


- [#11061](#11061)
[`74fb5d5c6`](74fb5d5)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed border radius
styling on `TextField.Spinner` component


- [#11062](#11062)
[`78ff4e665`](78ff4e6)
Thanks [@chloerice](https://github.com/chloerice)! - Updated
`ContextualSaveBar` icon to `RiskMajor` and updated logos in examples to
Shopify logo


- [#11052](#11052)
[`27317aa4b`](27317aa)
Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed destructive
states for PageActions and Page SecondaryActions


- [#11053](#11053)
[`caf553126`](caf5531)
Thanks [@laurkim](https://github.com/laurkim)! - Removed `experimental`
flag from Badge `tone` types


- [#11049](#11049)
[`7508e7014`](7508e70)
Thanks [@sophschneider](https://github.com/sophschneider)! - Fixed
Banner InlineIconBanner variant dismiss icon position when hideIcon is
true


- [#10808](#10808)
[`b0d5750b0`](b0d5750)
Thanks [@jesstelford](https://github.com/jesstelford)! - [IndexFilters]
Loading spinner moved to be a suffix within the search box.

- Updated dependencies
\[[`73b1d5d2c`](73b1d5d)]:
    -   @shopify/polaris-tokens@8.0.2

## @shopify/polaris-tokens@8.0.2

### Patch Changes

- [#10602](#10602)
[`73b1d5d2c`](73b1d5d)
Thanks [@patrickDouglas](https://github.com/patrickDouglas)! - Export
types missing for the Typescript Compiler (tsc) when using
Node16/Bundler Module Resolution.

## @shopify/stylelint-polaris@15.0.2

### Patch Changes

- Updated dependencies
\[[`73b1d5d2c`](73b1d5d)]:
    -   @shopify/polaris-tokens@8.0.2

## polaris.shopify.com@0.60.0

### Minor Changes

- [#11045](#11045)
[`a1c13938f`](a1c1393)
Thanks [@lgriffee](https://github.com/lgriffee)! - Add v12 polaris
migrator documentation

### Patch Changes

- [#10795](#10795)
[`e78051a80`](e78051a)
Thanks [@SMAKSS](https://github.com/SMAKSS)! - Fixed typos in `<Text />`
variant tokens


- [#11051](#11051)
[`f2fabff25`](f2fabff)
Thanks [@lgriffee](https://github.com/lgriffee)! - Alphabetize component
list in v11 to v12 migration guide


- [#11029](#11029)
[`ca67934e0`](ca67934)
Thanks [@lgriffee](https://github.com/lgriffee)! - Update
`v12-styles-replace-custom-property-color` migration with additional
tokens

- Updated dependencies
\[[`f81abddba`](f81abdd),
[`74fb5d5c6`](74fb5d5),
[`78ff4e665`](78ff4e6),
[`27317aa4b`](27317aa),
[`caf553126`](caf5531),
[`7508e7014`](7508e70),
[`73b1d5d2c`](73b1d5d),
[`b0d5750b0`](b0d5750)]:
    -   @shopify/polaris@12.0.2
    -   @shopify/polaris-tokens@8.0.2

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ascherkus pushed a commit to ascherkus/polaris that referenced this pull request Feb 19, 2025
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris-migrator@0.26.2

### Patch Changes

- [Shopify#11029](Shopify#11029)
[`ca67934e0`](Shopify@cdfc606)
Thanks [@lgriffee](https://github.com/lgriffee)! - Update
`v12-styles-replace-custom-property-color` migration with additional
tokens

- Updated dependencies
\[[`73b1d5d2c`](Shopify@8c8bdc0)]:
    -   @shopify/polaris-tokens@8.0.2
    -   @shopify/stylelint-polaris@15.0.2

## @shopify/polaris@12.0.2

### Patch Changes

- [Shopify#11037](Shopify#11037)
[`f81abddba`](Shopify@2c8209b)
Thanks [@chloerice](https://github.com/chloerice)! - Added tests for
`buttonFrom` and `buttonsFrom` utility functions


- [Shopify#11061](Shopify#11061)
[`74fb5d5c6`](Shopify@46467a7)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed border radius
styling on `TextField.Spinner` component


- [Shopify#11062](Shopify#11062)
[`78ff4e665`](Shopify@c8db6f6)
Thanks [@chloerice](https://github.com/chloerice)! - Updated
`ContextualSaveBar` icon to `RiskMajor` and updated logos in examples to
Shopify logo


- [Shopify#11052](Shopify#11052)
[`27317aa4b`](Shopify@b7c8685)
Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed destructive
states for PageActions and Page SecondaryActions


- [Shopify#11053](Shopify#11053)
[`caf553126`](Shopify@77e9c8b)
Thanks [@laurkim](https://github.com/laurkim)! - Removed `experimental`
flag from Badge `tone` types


- [Shopify#11049](Shopify#11049)
[`7508e7014`](Shopify@84aad13)
Thanks [@sophschneider](https://github.com/sophschneider)! - Fixed
Banner InlineIconBanner variant dismiss icon position when hideIcon is
true


- [Shopify#10808](Shopify#10808)
[`b0d5750b0`](Shopify@f68d5bb)
Thanks [@jesstelford](https://github.com/jesstelford)! - [IndexFilters]
Loading spinner moved to be a suffix within the search box.

- Updated dependencies
\[[`73b1d5d2c`](Shopify@8c8bdc0)]:
    -   @shopify/polaris-tokens@8.0.2

## @shopify/polaris-tokens@8.0.2

### Patch Changes

- [Shopify#10602](Shopify#10602)
[`73b1d5d2c`](Shopify@8c8bdc0)
Thanks [@patrickDouglas](https://github.com/patrickDouglas)! - Export
types missing for the Typescript Compiler (tsc) when using
Node16/Bundler Module Resolution.

## @shopify/stylelint-polaris@15.0.2

### Patch Changes

- Updated dependencies
\[[`73b1d5d2c`](Shopify@8c8bdc0)]:
    -   @shopify/polaris-tokens@8.0.2

## polaris.shopify.com@0.60.0

### Minor Changes

- [Shopify#11045](Shopify#11045)
[`a1c13938f`](Shopify@3bcae1f)
Thanks [@lgriffee](https://github.com/lgriffee)! - Add v12 polaris
migrator documentation

### Patch Changes

- [Shopify#10795](Shopify#10795)
[`e78051a80`](Shopify@23f1840)
Thanks [@SMAKSS](https://github.com/SMAKSS)! - Fixed typos in `<Text />`
variant tokens


- [Shopify#11051](Shopify#11051)
[`f2fabff25`](Shopify@8aedb53)
Thanks [@lgriffee](https://github.com/lgriffee)! - Alphabetize component
list in v11 to v12 migration guide


- [Shopify#11029](Shopify#11029)
[`ca67934e0`](Shopify@cdfc606)
Thanks [@lgriffee](https://github.com/lgriffee)! - Update
`v12-styles-replace-custom-property-color` migration with additional
tokens

- Updated dependencies
\[[`f81abddba`](Shopify@2c8209b),
[`74fb5d5c6`](Shopify@46467a7),
[`78ff4e665`](Shopify@c8db6f6),
[`27317aa4b`](Shopify@b7c8685),
[`caf553126`](Shopify@77e9c8b),
[`7508e7014`](Shopify@84aad13),
[`73b1d5d2c`](Shopify@8c8bdc0),
[`b0d5750b0`](Shopify@f68d5bb)]:
    -   @shopify/polaris@12.0.2
    -   @shopify/polaris-tokens@8.0.2

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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