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

Update visual grouping of layout buttons in topology control bar #3565

Merged
merged 1 commit into from Dec 12, 2019
Merged

Update visual grouping of layout buttons in topology control bar #3565

merged 1 commit into from Dec 12, 2019

Conversation

jeff-phillips-18
Copy link
Member

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 25, 2019
@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Nov 25, 2019
@jeff-phillips-18
Copy link
Member Author

/retest

andrewballantyne
andrewballantyne approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment •
edited

Choose a reason for hiding this comment

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

/lgtm

These changes look good.

I feel it a little strange that the layout buttons are not in a toolbar-item themselves though...
image

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019
@christianvogt
Copy link
Contributor

The hover state of the right button:
image

The hover state of the left button:
image

The shadows aren't consistent as the shadow overlaps in one case but not the other.

@serenamarie125 @mceledonia
Is this the correct look you want for the depressed state? IMO it doesn't stand out very well.

@Veethika
Copy link

Veethika commented Dec 4, 2019

The hover state of the right button:
image

The hover state of the left button:
image

The shadows aren't consistent as the shadow overlaps in one case but not the other.

@serenamarie125 @mceledonia
Is this the correct look you want for the depressed state? IMO it doesn't stand out very well.

@serenamarie125 @christianvogt @jeff-phillips-18 A few changes for the toggle visual:
• The shadow should apply to the whole toggle button and not parts of it
• To differentiate better between the hover and selected state for the toggle button, #4080 should be used for the hover state of the icon
(While not hovering over, the depressed state visual seems acceptable)

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Dec 5, 2019
@jeff-phillips-18
Copy link
Member Author

Updated to suggestions from @Veethika

X4m2F92Ebc

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 5, 2019
@andrewballantyne
Copy link
Contributor

X4m2F92Ebc

Hmmm, that looks odd to me. A drop shadow overlapping the other button feels weird in a "button group".

@jeff-phillips-18
Copy link
Member Author

@andrewballantyne Agreed, looking for UX feedback.

andrewballantyne
andrewballantyne suggested changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

The bot caught up to my permission change and added the approve label based on my approval. Just reverting that until we know more about the way this will look.

@openshift-ci-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2019
@christianvogt
Copy link
Contributor

We should provide a proper button group in PF that can be used here.

@jeff-phillips-18
Copy link
Member Author

@christianvogt Agreed and the PF team is aware:
https://github.com/patternfly/patternfly-next/issues/2478

@jeff-phillips-18
Copy link
Member Author

Updated visuals per discussions w/ @serenamarie125 :

FdtdKO7pDN

@serenamarie125
Copy link
Contributor

@sspeiche and I think this looks much better, @christianvogt @andrewballantyne do you agree? Unfortunately we didn't have any PF visuals folks available to review.

@andrewballantyne
Copy link
Contributor

The dark gray background is for just these two buttons (based on selection) or is it for all of them?

@jeff-phillips-18 @serenamarie125

@jeff-phillips-18
Copy link
Member Author

@andrewballantyne The dark grey background is only to represent the currently selected layout option.

@andrewballantyne
Copy link
Contributor

image

@jeff-phillips-18 Are we okay with this overlap?

cc @serenamarie125

@jeff-phillips-18
Copy link
Member Author

That overlap occurs when a layout button has focus and another is hovered over. Due to there being no space between the buttons.

@andrewballantyne
Copy link
Contributor

I'm good with the code / how it works as long as UX is okay with the overlap @jeff-phillips-18

@jeff-phillips-18
Copy link
Member Author

/retest

@jeff-phillips-18
Copy link
Member Author

WvHmSxav3y

@openshift/team-devconsole-ux Please review

serenamarie125
serenamarie125 approved these changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM! We may need to updated when PF provides design, but that timeline is unknown.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2019
@christianvogt
Copy link
Contributor

/lgtm

What's interesting is that I believe kiali is also doing their own customizations like this. We need to get into PF.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jeff-phillips-18, serenamarie125

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

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@jeff-phillips-18
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 581602d into openshift:master Dec 12, 2019
@spadgett spadgett added this to the v4.4 milestone Dec 16, 2019
@jeff-phillips-18 jeff-phillips-18 deleted the control-bar branch December 2, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewballantyne andrewballantyne andrewballantyne approved these changes

@serenamarie125 serenamarie125 serenamarie125 approved these changes

@gijohn gijohn Awaiting requested review from gijohn

@nemesis09 nemesis09 Awaiting requested review from nemesis09

Assignees

@andrewballantyne andrewballantyne

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jeff-phillips-18 @christianvogt @Veethika @andrewballantyne @serenamarie125 @openshift-ci-robot @openshift-bot @spadgett @openshift-merge-robot