Skip to content

UX changes and styling of replica set detail page #295

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
Merged
merged 1 commit into from
Jan 27, 2016
Merged

UX changes and styling of replica set detail page #295

merged 1 commit into from
Jan 27, 2016

Conversation

floreks
Copy link
Member

@floreks floreks commented Jan 26, 2016

Style update:

  • Changed sub-header height to 66px (same as toolbar)
  • Moved delete button up and changed text to Delete Replica Set
  • Adjusted vertical spacing between sidebar panel elements
  • Reverted accent pallete to blue (button color on replica set list page will be adjusted by @maciaszczykm)
  • Removed animation when switching tabs
  • Added No events found information when there are no events. (Please check if it looks ok)
  • Removed some tooltips messages from events table that are not very meaningful.

2 things still to do, but in other PRs:

  • Deleting service when deleting replica set. (Needs discussion)
  • Combining information about pod statuses.

@codecov-io
Copy link

Current coverage is 78.16%

Merging #295 into master will decrease coverage by -0.27% as of b68b1b8

@@            master    #295   diff @@
======================================
  Files           66      66       
  Stmts          510     513     +3
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            400     401     +1
  Partial          0       0       
- Missed         110     112     +2

Review entire Coverage Diff as of b68b1b8

Powered by Codecov. Updated on successful CI builds.

@bryk
Copy link
Contributor

bryk commented Jan 26, 2016

@maciaszczykm Can you review?

bryk
bryk reviewed Jan 26, 2016
View reviewed changes
src/app/frontend/replicasetdetail/replicasetdetail.html Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, I think this is the opposite. The previous version is correct. I think Ken wanted to change labels on the cards list, which I already done. Ref: https://msdn.microsoft.com/en-us/library/windows/desktop/bb246428(v=vs.85).aspx

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! There is even a documentation for that. :) I will change

@maciaszczykm
Copy link
Member

@bryk Yes, I'll check it.

maciaszczykm
maciaszczykm reviewed Jan 26, 2016
View reviewed changes
src/app/frontend/replicasetdetail/replicasetdetail.html Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This if statement doesn't cover empty events list case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@maciaszczykm
Copy link
Member

One issue with ng-if="ctrl.events", everything else LGTM.

@floreks
Copy link
Member Author

floreks commented Jan 26, 2016

PTAL

@maciaszczykm
Copy link
Member

LGTM

bryk
bryk reviewed Jan 26, 2016
View reviewed changes
src/app/frontend/replicasetdetail/replicasetdetail.scss Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just going with what the mock shows. It's not capitalized there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think we should be going with standard angular material components, unless it gives benefits for users to change them. With this we'll save engineering resources to implement features, rather than play with frameworks. Does this make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'm thinking the same. Looking at Ken's feedback felt like he wants view to be closer to mocks. That's why I've changed it.

@bryk
Copy link
Contributor

bryk commented Jan 26, 2016

A few discussion comments. PTAL.

@floreks floreks force-pushed the ux-styling branch 3 times, most recently from 5443c86 to 685a594 Compare January 26, 2016 14:46
floreks
floreks reviewed Jan 26, 2016
View reviewed changes
src/app/frontend/index.scss Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it ok to disable transition for tabs globally? I will add comment if it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment, of course :)

@floreks floreks force-pushed the ux-styling branch 3 times, most recently from a020222 to 4ebbf4a Compare January 27, 2016 07:49
floreks
floreks reviewed Jan 27, 2016
View reviewed changes
src/app/frontend/index.scss Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it ok if we disable tab switch animation globally?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but add a comment explaining what and why you're doing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course. Just wanted to make sure that it is ok.

@floreks
Copy link
Member Author

floreks commented Jan 27, 2016

@bryk @maciaszczykm PTAL

@bryk
Copy link
Contributor

bryk commented Jan 27, 2016

Last style comments and let's merge.

@floreks
Copy link
Member Author

floreks commented Jan 27, 2016

Done. PTAL.

@bryk
Copy link
Contributor

bryk commented Jan 27, 2016

Nice work, thanks! :)

bryk added a commit that referenced this pull request Jan 27, 2016
@bryk
Merge pull request #295 from floreks/ux-styling
bc11432 
UX changes and styling of replica set detail page
@bryk bryk merged commit bc11432 into kubernetes:master Jan 27, 2016
@floreks floreks deleted the ux-styling branch January 27, 2016 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@floreks @codecov-io @bryk @maciaszczykm @googlebot