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

Add collapse styles to default.less #9943

Merged
merged 8 commits into from Apr 19, 2018

Conversation

Projects
None yet
2 participants
@davidhatten
Contributor

davidhatten commented Apr 4, 2018

First of all, thank you for your contribution! :-)

Please makes sure that these checkboxes are checked before submitting your PR, thank you!

  • Make sure that you propose PR to right branch: bugfix for master, feature for latest active branch feature-x.x.
  • Make sure that you follow antd's code convention.
  • Run npm run lint and fix those errors before submitting in order to keep consistent code style.
  • Rebase before creating a PR to keep commit history clear.
  • Add some descriptions and refer relative issues for you PR.

Related Issue

This pushes color and padding for the Collapse component into defaults so they can easily be overridden.

I set up the values such that the Collapse will not change appearances for anyone currently using the defaults (or any values those defaults were using), but allows a user to remove all of the padding from both the header and the content of a Collapse.

Properties added to default.less:

@collapse-header-padding          
@collapse-header-bg
@collapse-content-padding         
@collapse-content-bg

I couldn't see any tests around default styles for any of the other components, but if these changes are testable I'll be happy to go back and add them.

I don't think this requires any updates to documentation.

Extra checklist:

* [ ] Update API docs for the component.
* [ ] Update/Add demo to demonstrate new feature.
* [ ] Update TypeScript definition for the component.
* [ ] Add unit tests for the feature.

@codecov

This comment has been minimized.

codecov bot commented Apr 4, 2018

Codecov Report

Merging #9943 into feature-3.5.0 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           feature-3.5.0    #9943   +/-   ##
==============================================
  Coverage          86.28%   86.28%           
==============================================
  Files                197      197           
  Lines               4740     4740           
  Branches            1321     1321           
==============================================
  Hits                4090     4090           
  Misses               647      647           
  Partials               3        3

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 74d81c2...57e3df7. Read the comment docs.

@davidhatten davidhatten changed the title from WIP Add collapse styles to default.less to Add collapse styles to default.less Apr 4, 2018

@@ -3,9 +3,6 @@

@collapse-prefix-cls: ~"@{ant-prefix}-collapse";

@collapse-header-bg: @background-color-light;

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

You shouldn't change the original variable name cause someone may use it already.

This comment has been minimized.

@davidhatten

davidhatten Apr 5, 2018

Contributor

Good catch. Fixed a few lines below.

// Collapse
// ---
@collapse-header-padding: 12px 0 12px 40px;
@collapse-header-background-color: #fafafa;

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

#fafafa => @background-color-light

@@ -3,8 +3,7 @@

@collapse-prefix-cls: ~"@{ant-prefix}-collapse";

@collapse-header-bg: @background-color-light;
@collapse-active-bg: @background-color-base;

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

same as @collapse-active-bg

This comment has been minimized.

@davidhatten

davidhatten Apr 5, 2018

Contributor

@collapse-active-bg isn't used anywhere. Unless there's some kind of logic auto-attaching it to something, it looks like @collapse-active-bg is just cruft. I can restore it if you'd like, but it's not doing anything and it would be cleaner to remove it than leave it around looking like it's doing something.

This comment has been minimized.

@davidhatten

davidhatten Apr 5, 2018

Contributor

Unless it's supposed to be attached to something, but I don't have insight into that intent or why it's no longer attached.

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

It is OK in that case.

// Collapse
// ---
@collapse-header-padding: 12px 0 12px 40px;
@collapse-header-background-color: @background-color-light;

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

use @collapse-header-bg

@collapse-header-padding: 12px 0 12px 40px;
@collapse-header-background-color: @background-color-light;
@collapse-content-padding: 0 @padding-md;
@collapse-content-background-color: @component-background;

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

@collapse-content-bg

@collapse-header-background-color: @background-color-light;
@collapse-content-padding: 0 @padding-md;
@collapse-content-background-color: @component-background;
@collapse-content-box-padding: @padding-md 0 @padding-md 0;

This comment has been minimized.

@afc163

afc163 Apr 5, 2018

Member

@collapse-content-box-padding and collapse-content-padding are a little confused. Just use a @collapse-padding: @padding-md and use it in this way:

& > &-box {
  padding-top: @collapse-padding;
  padding-bottom: @collapse-padding;
}

This comment has been minimized.

@davidhatten

davidhatten Apr 5, 2018

Contributor

You're right, that is confusing, thanks for the suggestion.

I would suggest making it @collapse-content-padding to differentiate from the header and then removing @collapse-content-box-padding.

I guess the problem is that left-right padding is handled by the content area, and top-down padding is handled with the box inside content, so I was trying to maintain that separation. I'll play with it more using your suggestions to try to make it simpler.

@davidhatten

This comment has been minimized.

Contributor

davidhatten commented Apr 6, 2018

How about this suggestion?

It's a little weird, because the @collapse-header-padding gets to be a one-liner, but the @collapse-content-padding-xxx is enumerated. I can't see a good way around this. The structure needs to match the original like that in order for the animation to be smooth.

@collapse-content-padding-top: @padding-md;
@collapse-content-padding-right: @padding-md;
@collapse-content-padding-bottom: @padding-md;
@collapse-content-padding-left: @padding-md;

This comment has been minimized.

@afc163

afc163 Apr 6, 2018

Member

Just @collapse-padding, I don't think we need to customize padding for each different direction.

This comment has been minimized.

@davidhatten

davidhatten Apr 6, 2018

Contributor

I would like that too, but see above comment.

If I don't put top/bottom padding on the inner box element inside component, the animation gets very chunky and gross.

This comment has been minimized.

@davidhatten

davidhatten Apr 6, 2018

Contributor

ah, but I didn't try putting all of the padding on the box instead of on the component until just now.

putting all the padding on the box works fine.

Making this change now.

@afc163

This comment has been minimized.

Member

afc163 commented Apr 8, 2018

The header background is different with demo now: https://ant.design/components/collapse/#header , check the borderless and Custom Panel demos.

@davidhatten

This comment has been minimized.

Contributor

davidhatten commented Apr 9, 2018

There we go, I checked the whole page again, this one should now result in an identical docs page.

@davidhatten davidhatten reopened this Apr 11, 2018

@afc163 afc163 merged commit 74d1c5e into ant-design:feature-3.5.0 Apr 19, 2018

4 checks passed

codecov/patch Coverage not affected when comparing 74d81c2...57e3df7
Details
codecov/project 86.28% remains the same compared to 74d81c2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk No dependency changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment