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(Collapse): fix nested collapse icon #421

Merged
merged 1 commit into from Mar 6, 2019

Conversation

jdkahn
Copy link
Contributor

@jdkahn jdkahn commented Mar 6, 2019

No description provided.

@jdkahn jdkahn requested a review from youluna March 6, 2019 09:20
@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #421 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #421      +/-   ##
==========================================
+ Coverage   90.82%   90.83%   +<.01%     
==========================================
  Files         241      241              
  Lines       13500    13501       +1     
  Branches     4160     4160              
==========================================
+ Hits        12262    12263       +1     
  Misses       1220     1220              
  Partials       18       18
Impacted Files Coverage Δ
src/collapse/panel.jsx 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 d39a9ca...3df45e8. Read the comment docs.

@youluna
Copy link
Member

youluna commented Mar 6, 2019

@tao1991123 please check this carefully! Will the PR have any bad impact on users? Someone who uses old css stylesheet and new js files

@jdkahn
Copy link
Contributor Author

jdkahn commented Mar 6, 2019

@tao1991123 Is it possible to do that? Wouldn't users have to use both styles and js files from the same version?

@youluna
Copy link
Member

youluna commented Mar 6, 2019

Wouldn't users have to use both styles and js files from the same version?

No, they don't. Users who use theme package( https://github.com/alibaba-fusion/next/blob/master/site/en-us/theme.md ) use both @alifd/next and @alifd/theme-xxx as their dependencies. @alifd/theme-xxx is generated based on a specific version of @alifd/next. @alifd/theme-xxx is published by users via Config Platform (fusion.design). The publish of @alifd/theme-xxx is less frequent than @alifd/next.

So version of css will always older than the js version( among the same minor verison ). Many users use it this way.

Users should keep css and js version in the same minor version.
e.g.
1.12.2 css with 1.12.2 js ---> Fusion ensure it works.
1.12.2 css with 1.12.6 js ---> Fusion ensure it works.
1.12.2 css with 1.13.6 js ---> Users should update the theme package

@tao1991123
Copy link
Contributor

@youluna means user writes css styles to overwrite the default style.

Only one case that users overwrite

.panel-expanded .icon {
xxxxx
}

but new style is priorer than the old

.panel .icon.icon-expand {
xxxx
}

I reviewed and I think it's ok, you remove none of old classnames, it compatible.
I mean there is nearly no user who did this.

so I approved

Copy link
Contributor

@tao1991123 tao1991123 left a comment

Choose a reason for hiding this comment

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

OK

@tao1991123 tao1991123 merged commit 7a6842a into alibaba-fusion:master Mar 6, 2019
@youluna
Copy link
Member

youluna commented Mar 6, 2019

close #416

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