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

UI: Add proxy icons to proxy services and instances where appropriate #5463

Merged
merged 1 commit into from Mar 22, 2019

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Mar 11, 2019

This PR adds new proxy icons to services with the Kind: connect-proxy

Screenshot 2019-03-11 at 10 26 19

In Service listing views:

Screenshot 2019-03-11 at 10 14 48

in Service detail/Instance listing views:

Screenshot 2019-03-11 at 10 15 00

and Instance detail views:

Screenshot 2019-03-11 at 10 19 08

An extra note here, the 'external service' icons will be changing to use a similar grey rounded box background and move to Type column in listing views. These icons will therefore move to use the new %type-icon CSS component in a later PR.

@johncowen johncowen requested a review from a team March 11, 2019 09:25
@johncowen johncowen added the theme/ui Anything related to the UI label Mar 11, 2019
Copy link

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

Code looks good, just had a couple non-blocking comments.

The vertical alignment of the Proxy badge looks a little off though.

image

I expected the proxy icon to line up with the kubernetes icon. Is this alignment intentional?

@@ -79,6 +80,15 @@
margin-top: -10px;
background-color: $color-transparent;
}
%with-proxy::before {
@extend %pseudo-icon;
background-image: url('data:image/svg+xml;charset=UTF-8,<svg width="18" height="18" xmlns="http://www.w3.org/2000/svg"><g fill-rule="nonzero" fill="none"><path d="M2.242 7.138l6.963.774a1 1 0 0 1 .883.883l.774 6.963a1 1 0 0 1-1.104 1.104l-6.963-.774a1 1 0 0 1-.883-.883l-.774-6.963a1 1 0 0 1 1.104-1.104z" stroke="%23BAC1CC" fill="%23FAFBFC"/><path d="M5.242 4.138l6.963.774a1 1 0 0 1 .883.883l.774 6.963a1 1 0 0 1-1.104 1.104l-6.963-.774a1 1 0 0 1-.883-.883l-.774-6.963a1 1 0 0 1 1.104-1.104z" stroke="%238E96A3" fill="%238E96A3"/><path d="M8.242 1.138l6.963.774a1 1 0 0 1 .883.883l.774 6.963a1 1 0 0 1-1.104 1.104l-6.963-.774a1 1 0 0 1-.883-.883l-.774-6.963a1 1 0 0 1 1.104-1.104z" stroke="%23BAC1CC" fill="%23FAFBFC"/></g></svg>');

Choose a reason for hiding this comment

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

This works fine, but it's a little gnarly having to account for the # encoding whenever adding an svg. Other UIs use ember-inline-svg to help treat svgs like svgs. ember-svg-jar is also popular.

Granted these are slightly different, since they put the svg in the DOM, but that also makes it possible to style the svg dynamically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are good options. If we were inlining SVGs into HTML I'd definitely use one of those, but given we're using inline CSS we can’t use them here.

text-indent: -9000px !important;
width: 24px;
margin-top: -8px;
transform: scale(0.7);

Choose a reason for hiding this comment

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

The scale here is a little odd. Is this because the SVG is too large? Why not set the width lower?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scale means I can change the scale of the entire icon in one line, otherwise I'd have to change the width/heights of 2 things in 2 selectors, the gray background which is CSS and the icon itself which is pseudo content, plus the size of the border radius. That would probably be about 10 lines of code as opposed to 1. Plus scale does what it says on the tin, and just seemed easier. Is there any reason why not to use scale here?

Choose a reason for hiding this comment

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

I don't think there's a reason not to aside from it being unexpected (with the expected thing being width/height) and maybe a "wtf" down the road if things don't line up and the transform: scale gets overlooked.

Not strong arguments and your reasoning is good, just thought I'd ask.

@johncowen
Copy link
Contributor Author

Yeah the vertical alignment is weird, I thought the same! I wasn't sure what to do as we don't have designs with this combination of icons. I did tweak external-source icon down a bit so the text and source icon was on the same baseline:

Screenshot 2019-03-22 at 16 57 01

But I kind of stopped there as I know these external-source icons are changing so figured it was pointless playing with it as the next change here will be those new icons, hopefully before the next release. I decided not to do that change in this PR to keep it separate, but pretty soon they'll all look like this:

Screenshot 2019-03-22 at 16 59 27

They all have the same gray box ^, so its more obvious how they line up. If we don't have time to do the new icons before next release, then I can shout @opihana to do me a quick mockup of how this should be aligned and we can do that pre-release instead.

I'm gonna merge this now, but thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants