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

fix: 4.0 table filters value may be previous #19873

Merged
merged 8 commits into from Nov 26, 2019
Merged

fix: 4.0 table filters value may be previous #19873

merged 8 commits into from Nov 26, 2019

Conversation

jeessy2
Copy link

@jeessy2 jeessy2 commented Nov 21, 2019

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

example:
https://codesandbox.io/s/elated-mendeleev-64277

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English 4.0 table filters value may be previous when use select, becanse of 'setFilteredKeys' is async
🇨🇳 Chinese select选中一个option, 触发的搜索是前一个值 ,因’setFilteredKeys‘是异步的

☑️ Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Could you please add tests to make sure this change works as expected?

@netlify
Copy link

netlify bot commented Nov 21, 2019

Deploy preview for ant-design ready!

Built with commit 20a6540

https://deploy-preview-19873--ant-design.netlify.com

@jeessy2
Copy link
Author

jeessy2 commented Nov 21, 2019

@zombieJ added a test case

@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #19873 into 4.0-prepare will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           4.0-prepare   #19873      +/-   ##
===============================================
+ Coverage        97.49%   97.49%   +<.01%     
===============================================
  Files              291      292       +1     
  Lines             7231     7239       +8     
  Branches          1957     1996      +39     
===============================================
+ Hits              7050     7058       +8     
  Misses             181      181
Impacted Files Coverage Δ
...omponents/table/hooks/useFilter/FilterDropdown.tsx 95.89% <100%> (ø) ⬆️
components/table/hooks/useSyncState.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 060b7de...20a6540. Read the comment docs.

@zombieJ
Copy link
Member

zombieJ commented Nov 22, 2019

Create a hook name useSyncState to handle this:

const [getFilteredKeys, setFilteredKeys] = React.useState(propFilteredKeys || []);

// ...

const onConfirm = () => {
  internalTriggerFilter(getFilteredKeys());
};

@jeessy2
Copy link
Author

jeessy2 commented Nov 22, 2019

Create a hook name useSyncState to handle this:

const [getFilteredKeys, setFilteredKeys] = React.useState(propFilteredKeys || []);

// ...

const onConfirm = () => {
  internalTriggerFilter(getFilteredKeys());
};

getFilteredKeys(), It's not a function

@yoyo837
Copy link
Contributor

yoyo837 commented Nov 22, 2019

const [filteredKeys, setFilteredKeys] = React.useState(propFilteredKeys || []);

// ...

const onConfirm = () => {
  internalTriggerFilter(filteredKeys);
};

@jeessy2
Copy link
Author

jeessy2 commented Nov 22, 2019

const [filteredKeys, setFilteredKeys] = React.useState(propFilteredKeys || []);

// ...

const onConfirm = () => {
  internalTriggerFilter(filteredKeys);
};

Use this can't fix the bug

@yoyo837
Copy link
Contributor

yoyo837 commented Nov 22, 2019

getFilteredKeys(), It's not a function

I just correct this code errors.

@jeessy2
Copy link
Author

jeessy2 commented Nov 22, 2019

create a sync variable to resolve async 'setFilteredKeys'

 let filteredKeysLast = filteredKeys;

@zombieJ
Copy link
Member

zombieJ commented Nov 22, 2019

I mean to create a hook your own to handle these code logic:

// useSyncState.tsx

export default function useSyncState(...) {
  const stateRef = React.useRef(...);
  // ...
  return [
    () => stateRef.current;
    (newValue) => ...
  ];
}

getXXX called in onConfirm will always get synced state value.

@jeessy2
Copy link
Author

jeessy2 commented Nov 22, 2019

I mean to create a hook your own to handle these code logic:

// useSyncState.tsx

export default function useSyncState(...) {
  const stateRef = React.useRef(...);
  // ...
  return [
    () => stateRef.current;
    (newValue) => ...
  ];
}

getXXX called in onConfirm will always get synced state value.

fix to this


return [
() => filteredKeysRef.current,
(newValue: T) => (filteredKeysRef.current = newValue),
Copy link
Member

Choose a reason for hiding this comment

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

Update filter should trigger re-render:

const [_, forceUpdate] = React.useState();
// ...
(newValue: T) => {
  filteredKeysRef.current = newValue;
  forceUpdate...
}

@zombieJ zombieJ merged commit 221d404 into ant-design:4.0-prepare Nov 26, 2019
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.

None yet

3 participants