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

feat: responsive table #23298

Merged
merged 1 commit into from Apr 24, 2020
Merged

Conversation

vbudovski
Copy link
Contributor

@vbudovski vbudovski commented Apr 15, 2020

🤔 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

Has been requested/attempted several times.
#11817

💡 Background and solution

Large Ant tables do not display well on smaller screens. Allow selective display of columns based on media query check in React.

📝 Changelog

Columns are only filtered out in the presence of a responsive attribute.

Language Changelog
🇺🇸 English Table support responsive columns.
🇨🇳 Chinese Table 支持响应式展现列。

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • 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

View rendered CHANGELOG.en-US.md
View rendered CHANGELOG.zh-CN.md
View rendered components/empty/index.en-US.md
View rendered components/form/index.en-US.md
View rendered components/form/index.zh-CN.md
View rendered components/select/index.en-US.md

@vbudovski vbudovski requested a review from zombieJ as a code owner Apr 15, 2020
@ant-design-bot
Copy link
Contributor

@ant-design-bot ant-design-bot commented Apr 15, 2020

@codesandbox-ci
Copy link

@codesandbox-ci codesandbox-ci bot commented Apr 15, 2020

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 6483500:

Sandbox Source
antd reproduction template Configuration

@yoyo837
Copy link
Contributor

@yoyo837 yoyo837 commented Apr 16, 2020

Commits should be clean!

@vbudovski vbudovski force-pushed the feat/responsive-table branch 2 times, most recently from 34a0799 to 42b3a39 Compare Apr 16, 2020
@yoyo837 yoyo837 changed the title Feat/responsive table feat: responsive table Apr 16, 2020
@zombieJ
Copy link
Member

@zombieJ zombieJ commented Apr 16, 2020

Grid already realize the responsive detect. Should not re-import another lib: https://github.com/ant-design/ant-design/blob/cff24d5dcb03207bf587d454e56bd2d477c0b555/components/grid/row.tsx

@codecov
Copy link

@codecov codecov bot commented Apr 16, 2020

Codecov Report

Merging #23298 into feature will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           feature   #23298   +/-   ##
========================================
  Coverage    98.35%   98.35%           
========================================
  Files          365      365           
  Lines         7290     7290           
  Branches      2000     1952   -48     
========================================
  Hits          7170     7170           
  Misses         120      120           
Impacted Files Coverage Δ
components/table/hooks/useFilter/index.tsx 100.00% <ø> (ø)
components/table/hooks/useSorter.tsx 96.07% <ø> (-0.06%) ⬇️
components/table/Table.tsx 99.22% <100.00%> (+0.02%) ⬆️

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 87da7bf...6483500. Read the comment docs.

@vbudovski
Copy link
Contributor Author

@vbudovski vbudovski commented Apr 16, 2020

@zombieJ Fixed. It's a lot cleaner now.

components/table/demo/responsive.md Outdated Show resolved Hide resolved
components/table/demo/responsive.md Outdated Show resolved Hide resolved
components/table/Table.tsx Outdated Show resolved Hide resolved
components/table/Table.tsx Outdated Show resolved Hide resolved
components/table/Table.tsx Outdated Show resolved Hide resolved
@vbudovski vbudovski requested a review from zombieJ Apr 19, 2020
@zombieJ
Copy link
Member

@zombieJ zombieJ commented Apr 19, 2020

Thanks for your effort. I'm OK with the code, could you help to update table document about this props?

@afc163 @yoyo837, pls help double confirm for the prop name of responsive

@vbudovski
Copy link
Contributor Author

@vbudovski vbudovski commented Apr 19, 2020

I realised that useBreakpoint works a bit differently to the other library. We can probably simplify the interface to take a single instance of Breakpoint rather than an array. What do you think @zombieJ ?

@yoyo837
Copy link
Contributor

@yoyo837 yoyo837 commented Apr 20, 2020

@vbudovski You can send a new PR about that to help improve.

afc163
afc163 approved these changes Apr 20, 2020
@afc163 afc163 merged commit f09686d into ant-design:feature Apr 24, 2020
19 checks passed
@afc163
Copy link
Member

@afc163 afc163 commented May 7, 2020

@vbudovski Could you fill API documentation about responsive?

https://ant.design/components/table/#Column
https://ant.design/components/table-cn/#Column

@muzea
Copy link
Contributor

@muzea muzea commented May 7, 2020

const mergedColumns = React.useMemo(() => {
const matched = new Set(Object.keys(screens).filter((m: Breakpoint) => screens[m]));
return (columns || convertChildrenToColumns(children)).filter(
(c: ColumnType<RecordType>) =>
!c.responsive || c.responsive.some((r: Breakpoint) => matched.has(r)),
);
}, [children, columns, screens]);

Is it inappropriate for children to be dependent? In my case, it will cause a lot of extra render.

@afc163
Copy link
Member

@afc163 afc163 commented May 8, 2020

@muzea send PR to optimize it.

@zombieJ
Copy link
Member

@zombieJ zombieJ commented May 8, 2020

Is it inappropriate for children to be dependent? In my case, it will cause a lot of extra render.

children is only used when define Columns by jsx. It's empty when use columns.

@vbudovski vbudovski deleted the feat/responsive-table branch May 9, 2020
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

6 participants