Skip to content

Conversation

alllx
Copy link
Contributor

@alllx alllx commented Mar 9, 2020

WHY are these changes introduced?

Because of unexpected search options result was displayed like unexpected options result or duplicated options (see animation below).
2020-03-03 18 18 40

Fixes #2764

WHAT is this pull request doing?

Prevents passed component props mutation.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@ghost
Copy link

ghost commented Mar 9, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@alllx alllx requested review from AndrewMusgrave and BPScott March 9, 2020 16:02
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2020

🟢 This pull request modifies 3 files and might impact 1 other files.

Details:
All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Autocomplete/components/ComboBox/ComboBox.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/Autocomplete/tests/Autocomplete.test.tsx (total: 0)

Files potentially affected (total: 0)

@alllx alllx force-pushed the fix/#2764-prevent-props-mutation branch 2 times, most recently from 67bb0fb to 17479ef Compare March 10, 2020 10:30
@alllx alllx force-pushed the fix/#2764-prevent-props-mutation branch from 17479ef to e6f3748 Compare March 10, 2020 11:12
@alllx alllx mentioned this pull request Mar 12, 2020
6 tasks
@dleroux
Copy link
Contributor

dleroux commented Mar 12, 2020

How were you able to tophat your actionsBefore and actionsAfter changes?

actionsBefore throws an error: Maximum update depth exceeded, and I'm seeing actionsAfter. If you got this to work can you provide a playground?

@BPScott BPScott removed their request for review March 12, 2020 16:43
@BPScott
Copy link
Member

BPScott commented Mar 12, 2020

I'm not familiar with AutoComplete, so I'm probably not the best person to review this, looks like @dleroux has got your back though

@alllx
Copy link
Contributor Author

alllx commented Mar 12, 2020

How were you able to tophat your actionsBefore and actionsAfter changes?

actionsBefore throws an error: Maximum update depth exceeded, and I'm seeing actionsAfter. If you got this to work can you provide a playground?

Let me double check this in Playground, there is a test which is passed: src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx:191

@alllx
Copy link
Contributor Author

alllx commented Mar 12, 2020

I'm not familiar with AutoComplete, so I'm probably not the best person to review this, looks like @dleroux has got your back though

Hey @dleroux, I checked actionBefore on Polaris master branch, same error, so I didn't introduce this error. This issue should be tackled in the separate PR.
Here is my Playground.tsx code please check it in storybook on master.

import React from 'react';
import {CirclePlusMinor} from '@shopify/polaris-icons';

import {Page, Autocomplete} from '../src';

export function Playground() {
  const textField = (
    <Autocomplete.TextField
      onChange={(value) => console.log(value)}
      label=""
      value=""
      placeholder="placeholder"
    />
  );
  return (
    <Page title="Playground">
      <Autocomplete
        options={[
          {
            label: 'label 1',
            value: '1',
          },
          {
            label: 'label 2',
            value: '2',
          },
        ]}
        selected={['1']}
        onSelect={() => console.log('update')}
        textField={textField}
        actionBefore={{
          content: "Add 'f'",
          icon: CirclePlusMinor,
          id: 'ComboBox3-0',
        }}
      />
    </Page>
  );
}

@alllx alllx requested a review from dleroux March 12, 2020 17:56
@dleroux
Copy link
Contributor

dleroux commented Mar 12, 2020

The code looks good, but mine keeps crashing, even with your playground on master. I've reopened the issue that was closed. Hopefully someone has time to tackle it soon but that's another issue.

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

I'm approving based on the code but hopefully someone else can tophat that prop can do the same. @AndrewMusgrave ?

@dleroux dleroux merged commit 5bcf92b into master Mar 13, 2020
@dleroux dleroux deleted the fix/#2764-prevent-props-mutation branch March 13, 2020 13:01
@ghost
Copy link

ghost commented Mar 13, 2020

🎉 Thanks for your contribution to Polaris React!

@tmlayton tmlayton temporarily deployed to production March 14, 2020 00:47 Inactive
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.

Displaying Incorrect Suggested Tags in Multiple tag autocomplete

4 participants