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 - Show/search by service ids #4387

Merged
merged 3 commits into from
Jul 13, 2018
Merged

UI - Show/search by service ids #4387

merged 3 commits into from
Jul 13, 2018

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jul 12, 2018

Show Service.ID's throughout the app, allow searching by Service.ID

  1. In the Services > Services detail page for both healthy and unhealthy
    nodes, also add searching by Service.ID here

Before:

nodes-before

After:

nodes-after-2

  1. In the Nodes > Node detail > [Services] tab only if its different
    from the Service name, add searching by Service.ID here

Before:

services-before

After:

services-after

Fixes: #4153

John Cowen added 3 commits July 12, 2018 13:35
1. In the Services > Services detail page for both healthy and unhealthy
nodes, also add searching by Service.ID here
2. In the Nodes > Node detail > [Services] tab only if its different
from the Service name, add searching by Service.ID here
@johncowen johncowen requested a review from a team July 12, 2018 15:30
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.

Just some questions, the change looks good. It sounds like it'll make some users happy too.

}
.unhealthy .healthchecked-resource header.with-service a {
padding-bottom: 25px;
}

Choose a reason for hiding this comment

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

Why don't these use placeholder selectors? Doesn't that make these selectors dependent on the name of the class that extends the placeholder and thus sorta defeat the purpose of the placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah defo 👍 I'll be splitting them into something like %healthchecked-resource, %unhealthy-resource and %healthy-resource I think. Until yesterday I was in 'tweak-mode', so this will get revisited as part of a larger CSS refactor now I'm back to more 'heavy-handed mode'

@@ -35,7 +35,7 @@
<h2>Healthy Nodes</h2>
{{#list-collection
items=healthy
cell-layout=(percentage-columns-layout healthy.length columns 100) as |item index|
cell-layout=(percentage-columns-layout healthy.length columns 92) as |item index|

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed the vertical margins between the unhealthy nodes and the healthy nodes where different by 4px (22px vs 26px), so I equalled them up.

Before:

before-2

After:

after-2

Ideally they'd be the same, which reminds me I need to ask @opihana about the vertical rhythm of everything across the entire app

@@ -49,12 +50,13 @@
<h2>Healthy Nodes</h2>
{{#list-collection
items=healthy
cell-layout=(percentage-columns-layout healthy.length columns 100) as |item index|
cell-layout=(percentage-columns-layout healthy.length columns 113) as |item index|

Choose a reason for hiding this comment

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

Another peculiar number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, so similar to the above its to do with maintaining the 22px margin, you have to specify a row height for the DOM recycling scrollers, and as the boxes grew to make room for the Server.ID this number needed to grow also. Would be nice to auto calculate I suppose, but its a bit 'chicken and egg' 🐔 🥚 (gotta love an emoji 😄 )

s: service-0-with-id
---
And I see 1 [Model] model
And I see 1 [Model] model with the id "service-0-with-id"

Choose a reason for hiding this comment

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

Is this ID hardcoded in the API double?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johncowen johncowen merged commit 7202f6e into master Jul 13, 2018
@johncowen johncowen deleted the feature/ui-service-ids branch July 13, 2018 08:38
@johncowen johncowen added the theme/ui Anything related to the UI label Nov 29, 2019
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.

Service ID is hidden in the new UI.
2 participants