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(module:steps): remove top-level redundant div element #7582

Merged
merged 1 commit into from Aug 4, 2022
Merged

fix(module:steps): remove top-level redundant div element #7582

merged 1 commit into from Aug 4, 2022

Conversation

HyperLife1119
Copy link
Collaborator

@HyperLife1119 HyperLife1119 commented Jul 28, 2022

The first div child of the nz-steps element is redundant and should be merged with the nz-steps element.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

QT_9R @YRGM{ZQ0V94{G06G

There is a layer of redundant div element under the nz-steps element.

Issue Number: N/A

What is the new behavior?

E`UNOE7QB6639RT66@ FQF2

Remove redundant div element.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

I'm not sure if this introduces breaking changes, but this changes the existing DOM structure.

Other information

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #7582 (a21c380) into master (f18149a) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head a21c380 differs from pull request most recent head 355a788. Consider uploading reports for the commit 355a788 to get more accurate results

@@            Coverage Diff             @@
##           master    #7582      +/-   ##
==========================================
- Coverage   91.74%   91.73%   -0.01%     
==========================================
  Files         502      502              
  Lines       16756    16746      -10     
  Branches     2757     2755       -2     
==========================================
- Hits        15372    15362      -10     
  Misses       1050     1050              
  Partials      334      334              
Impacted Files Coverage Δ
components/steps/demo/nav.ts 50.00% <ø> (ø)
components/steps/steps.component.ts 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Member

@hsuanxyz hsuanxyz left a comment

Choose a reason for hiding this comment

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

LGTM, but there has been a breaking change, just like the following screenshot, if someone used the display attribute to nz-steps his style will be broken. I suggest to merge it in v14.

image

The first `div` child of the `nz-steps` element is redundant and should be merged with the `nz-steps` element.
@HyperLife1119 HyperLife1119 changed the base branch from master to 14.0.x July 28, 2022 12:47
@HyperLife1119 HyperLife1119 changed the base branch from 14.0.x to master July 28, 2022 12:49
@HyperLife1119
Copy link
Collaborator Author

I just fixed the demo, do I need to point this PR to the v14 branch?

@hsuanxyz
Copy link
Member

thx, no, I'll label it.

@hsuanxyz hsuanxyz added the 💔 Breaking Change This PR or the solution to this issue would introduce breaking changes label Jul 29, 2022
@simplejason simplejason merged commit 60beabc into NG-ZORRO:master Aug 4, 2022
@pr-triage pr-triage bot added the PR: merged label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💔 Breaking Change This PR or the solution to this issue would introduce breaking changes PR: merged PR: reviewed-approved PR: target-major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants