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

Collapse extended options wizard #1609

Merged
merged 32 commits into from
Oct 22, 2019

Conversation

kgroschoff
Copy link
Contributor

@kgroschoff kgroschoff commented Sep 25, 2019

What this PR does / why we need it:
Add possibility to collapse extended options & restructure order,

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1518

Special notes for your reviewer:
Sorry for this huge PR 😞

I would suggest to use the same styling for Provider Tags as vor Node Labels. But as this will be definitely too much for this PR, I'd keep this as seperate issue.

Extremely short overview:

  • SetSettingsComponent includes ExtendedOptionsComponent
  • ExtendedOptionsComponent includes ClusterProviderOptionsComponent & NodeDataOptionsComponent

Example AWS:
extopt1

Release note:

Redesign extended options in wizard

@kgroschoff kgroschoff self-assigned this Sep 25, 2019
@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 25, 2019
@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kgroschoff

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2019
@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #1609 into master will decrease coverage by 0.05%.
The diff coverage is 59.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1609      +/-   ##
==========================================
- Coverage   71.57%   71.52%   -0.06%     
==========================================
  Files         210      222      +12     
  Lines        6818     7164     +346     
  Branches      682      712      +30     
==========================================
+ Hits         4880     5124     +244     
- Misses       1671     1764      +93     
- Partials      267      276       +9
Impacted Files Coverage Δ
...ata/packet-node-data/packet-node-data.component.ts 58.02% <ø> (+0.55%) ⬆️
src/app/shared/entity/NodeEntity.ts 72.91% <ø> (ø) ⬆️
.../app/wizard/set-settings/set-settings.component.ts 71.42% <0%> (ø) ⬆️
...node-data/gcp-node-data/gcp-node-data.component.ts 41.46% <100%> (-0.39%) ⬇️
...ta/vsphere-add-node/vsphere-node-data.component.ts 92.3% <100%> (ø) ⬆️
...a/node-data-options/node-data-options.component.ts 100% <100%> (ø)
src/app/shared/utils/common-utils.ts 71.42% <100%> (+7.79%) ⬆️
...ngs/provider-options/provider-options.component.ts 100% <100%> (ø)
...node-data/aws-node-data/aws-node-data.component.ts 67.54% <100%> (+2.48%) ⬆️
...ider-options/vsphere-provider-options.component.ts 35.87% <35.87%> (ø)
... and 39 more

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 763f04d...63c7e7b. Read the comment docs.

@kgroschoff kgroschoff changed the title [WIP] Collapse extended options wizard Collapse extended options wizard Sep 25, 2019
@kubermatic-bot kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2019
@kgroschoff kgroschoff requested review from maciaszczykm and floreks and removed request for maciaszczykm September 25, 2019 15:04
@kgroschoff
Copy link
Contributor Author

kgroschoff commented Sep 26, 2019

/hold
found a issue that only appears in this branch with openstack (when using presets), will have a look

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2019
@kgroschoff
Copy link
Contributor Author

/hold cancel

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2019
@kgroschoff kgroschoff force-pushed the feature/collapse-extended-options-wizard branch from 45b445b to 88538c2 Compare September 26, 2019 12:37
@@ -0,0 +1,3 @@
.km-tags-in-wizard {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it works for all kind of tags then can we move to the main app.scss?

key: new FormControl(''),
value: new FormControl(''),
}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a separate function for tag list initialization?

} else {
this.azureNodeForm.controls.size.enable();
this.form.controls.size.enable();
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

</div>

<div class="km-form-group-tags-header">
Tags
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the tags for Azure and for example AWS work in the same way? This is not the scope of this PR but we could do it in another one and it would reduce code duplication a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and I'd like to style them similar to e.g. Node Labels. So probably a new component for Provider Tags, but I'd like to do this in a seperate PR, as this also will be a bigger PR.

key: new FormControl(''),
value: new FormControl(''),
}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we separate to another method?

@kgroschoff kgroschoff force-pushed the feature/collapse-extended-options-wizard branch from 88538c2 to c236cf3 Compare October 14, 2019 10:40
@kubermatic-bot kubermatic-bot added the dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. label Oct 14, 2019
@kgroschoff
Copy link
Contributor Author

@maciaszczykm PTAL :)

@kgroschoff kgroschoff force-pushed the feature/collapse-extended-options-wizard branch from eb1cfa2 to aed130b Compare October 18, 2019 07:08
@kgroschoff
Copy link
Contributor Author

@maciaszczykm I resolved the merge conflicts. PTAL

@maciaszczykm
Copy link
Contributor

@kgroschoff Sorry, for the delay... I am looking on it right now.

@maciaszczykm
Copy link
Contributor

/lgtm

Great work 👏

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2019
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a0987f1be79f41ebce045b2c285228abc7ef33d7

@kubermatic-bot kubermatic-bot merged commit 858a2d5 into master Oct 22, 2019
@kubermatic-bot kubermatic-bot deleted the feature/collapse-extended-options-wizard branch October 22, 2019 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to collapse extended options in wizard
3 participants