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.

Sign up for GitHub

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

Jump to bottom

Autoscaler updates following UX review #3817

Merged
merged 4 commits into from
Sep 3, 2019
Merged

Autoscaler updates following UX review #3817

merged 4 commits into from
Sep 3, 2019

Conversation

richard-cox
Copy link
Contributor

@richard-cox richard-cox commented Aug 30, 2019 •
edited
Loading

  • Move create/edit/delete policy buttons into tab header
  • Show standard 'no content' style message when no policy attached
  • Show scaling history only when there's history OR there's a policy
  • Don't show error snack bar for no policy
  • Tweak card layouts and widths
  • Tweak some error messages
  • Add brief text for each step
  • Show Create/Edit in stepper given create/edit

Update

  • Determine if app autoscaler tab should show depending on autoscaler v1/info request rather than
    a per app request for it's policy
  • Also show autoscaler version in cf summary page

@richard-cox
Autoscaler updates following UX review
13fe609 
- Move create/edit/delete policy buttons into tab header
- Show standard 'no content' style message when no policy attached
- Show scaling history only when there's history OR there's a policy
- Don't show error snack bar for no policy
- Tweak card layouts and widths
- Tweak some error messages
- Add brief text for each step
- Show Create/Edit in stepper given create/edit
@richard-cox richard-cox self-assigned this Aug 30, 2019
@cfdreddbot
Copy link

✅ Hey richard-cox! The commit authors and yourself have already signed the CLA.

@richard-cox
Show autoscaler tab depending on autoscaler v1/info endpoint
730e4d2 
Also show autoscaler version in cf summary page
@codecov
Copy link

codecov bot commented Sep 2, 2019 •
edited
Loading

Codecov Report

Merging #3817 into v2-master will increase coverage by 0.03%.
The diff coverage is 48.07%.

@@              Coverage Diff              @@
##           v2-master    #3817      +/-   ##
=============================================
+ Coverage      52.15%   52.19%   +0.03%     
=============================================
  Files            784      785       +1     
  Lines          22960    22983      +23     
  Branches        4109     4115       +6     
=============================================
+ Hits           11974    11995      +21     
- Misses         10986    10988       +2

vitoravelino
vitoravelino reviewed Sep 2, 2019
View reviewed changes
src/frontend/packages/cf-autoscaler/src/store/autoscaler.effects.ts Outdated
return this.http
.request(new Request(options)).pipe(
mergeMap(response => {
console.log(response);
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover.

@vitoravelino
Copy link
Contributor

vitoravelino commented Sep 2, 2019 •
edited
Loading

I was able to verify the mentioned points above were implemented. I was also able to find some issues: (I've used start-dev)

  • Immutability issue when clicking on Next when editing the policy (e.g.: this.currentPolicy.schedules.timezone = ...);
  • Immutability issue when clicking on Finish when editing the policy (e.g.: request.error = false);
  • Threshold label (mb, ms, rps, ...) does not change in the sentence when changing the metric type (after finishing it changes but not when creating);

@vitoravelino vitoravelino added the needs attention This PR needs attention label Sep 2, 2019
@richard-cox
Fix issues from review
023500e 
- start-dev/immutable object issue to discuss
@richard-cox
Copy link
Contributor Author

I've fixed the issues mentioned, however want to run through the start-dev/immutable object issue with @KlapTrap tomorrow.

@vitoravelino vitoravelino removed the needs attention This PR needs attention label Sep 3, 2019
vitoravelino
vitoravelino approved these changes Sep 3, 2019
View reviewed changes
Copy link
Contributor

@vitoravelino vitoravelino left a comment

Choose a reason for hiding this comment

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

LGTM

@richard-cox richard-cox merged commit 9d37b6a into v2-master Sep 3, 2019
@richard-cox richard-cox deleted the autoscaler-tweaks branch September 3, 2019 12:27
@richard-cox richard-cox restored the autoscaler-tweaks branch September 3, 2019 12:31
@richard-cox richard-cox mentioned this pull request Sep 3, 2019
Merged
@nwmac nwmac deleted the autoscaler-tweaks branch April 10, 2020 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitoravelino vitoravelino vitoravelino approved these changes

Assignees

@richard-cox richard-cox

Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@richard-cox @cfdreddbot @vitoravelino