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

Make breakpoints use the token feature #39105

Merged
merged 25 commits into from Dec 15, 2022

Conversation

azro352
Copy link
Contributor

@azro352 azro352 commented Nov 29, 2022

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature

🔗 Related issue link

close #32954
close #34197
close #19694
close #22359

💡 Background and solution

Screens are getting bigger, we need a new size to handle them. For example with 4k screens, you can differenciate an half-screen of a full-screen (so can't adapt the width f you app only for full-screen mode)

📝 Changelog

Language Changelog
🇺🇸 English Breakpoints can now follow theme token config.
🇨🇳 Chinese Breakpoints 现在会消费主题 token 配置。

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

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

The decision has been made to use the new token system for the breakpoints

I have decided to remove ResponsiveObserve.unsubscribe should be called when unmounted test because now we use a hook, that isn't the same instance of ResponsiveObserver that is shared between Row and the test, so we can't check the call on subscribe at this moment

Thanks

@github-actions
Copy link
Contributor

github-actions bot commented Nov 29, 2022

@yoyo837
Copy link
Contributor

yoyo837 commented Nov 30, 2022

Maybe feature branch is better.

@yoyo837
Copy link
Contributor

yoyo837 commented Nov 30, 2022

CI failed.

@yoyo837
Copy link
Contributor

yoyo837 commented Nov 30, 2022

feature branch please!

@azro352 azro352 changed the base branch from master to feature November 30, 2022 07:24
@azro352

This comment was marked as resolved.

@yoyo837
Copy link
Contributor

yoyo837 commented Nov 30, 2022

feature branch please!

You said "maybe" so I wasn't sure If I needed to change or not , sorry :)

My bad. :)

yoyo837
yoyo837 previously requested changes Dec 1, 2022
Copy link
Contributor

@yoyo837 yoyo837 left a comment

Choose a reason for hiding this comment

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

So sorry, The owner think that this should be supported by adding some style token by ConfigProvider with theme. #39105 (comment)
https://ant.design/docs/react/customize-theme#customize-design-token

@azro352 azro352 requested review from zombieJ and afc163 and removed request for zombieJ and afc163 December 1, 2022 11:02
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (c5e91b8) compared to base (49e8c13).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##           feature    #39105   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          556       556           
  Lines         9595      9602    +7     
  Branches      2714      2714           
=========================================
+ Hits          9595      9602    +7     
Impacted Files Coverage Δ
components/_util/responsiveObserve.ts 100.00% <100.00%> (ø)
components/descriptions/index.tsx 100.00% <100.00%> (ø)
components/grid/hooks/useBreakpoint.tsx 100.00% <100.00%> (ø)
components/grid/row.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -88,15 +83,6 @@ describe('Grid', () => {
expect(asFragment().firstChild).toMatchSnapshot();
});

it('ResponsiveObserve.unsubscribe should be called when unmounted', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Do not remove directly which should replace with new test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do not remove existing test cases, add some new test cases for this feature directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yoyo837 @zombieJ

I have no idea how to replace it. What do I do ? Who write the new test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yoyo837 @zombieJ
Tell me at least, the behaviour you expect to be tested ? And I will try to think about the test implementation

Copy link
Member

Choose a reason for hiding this comment

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

Let me handle this : )

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.

响应式栅格可否增加1920的尺寸?
6 participants