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

[Graph] Style fixes #47667

Merged
merged 3 commits into from Oct 10, 2019
Merged

[Graph] Style fixes #47667

merged 3 commits into from Oct 10, 2019

Conversation

flash1293
Copy link
Contributor

Summary

This PR fixes various styling issues in Graph, partially introduced by the migration, partially already existing in legacy UI.

The comments in the code explain what the various changes are doing.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@flash1293 flash1293 added Feature:Graph Graph application feature v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 9, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@@ -22,6 +22,8 @@
}

.gphGraph__container {
display: flex;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By making this a flex container, the svg grows to the correct size in Safari (otherwise it falls back to the default height of 150px)

@@ -27,7 +27,7 @@

.gphSidebar__panel{
max-height: $euiSizeL * 10;
overflow-y: auto;
overflow-y: hidden auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow setting overflow-y to auto also set overflow-x to auto which causes a horizontal scroll bar here because of the way the bootstrap forms are working.

@@ -49,6 +49,10 @@
margin-bottom: 0;
}

.gphSelectionList__icon {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the circle in the sidebar selection list is much smaller, the icon should also be smaller, otherwise the icon overlaps the circle in some cases.

@@ -7,7 +7,8 @@
<graph-inspect ng-show="menus.showInspect"></graph-inspect>
</div>

<graph-app
<div
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safari and IE don't like the custom elements (like graph-app) in the DOM - by using attribute directives here they behave like regular divs.

x="12"
y="16"
ng-click="workspace.deselectNode(n)"
ng-class="{'gphNode__text--inverse': isColorDark(n.color)}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the additional class to change the icon color if the vertex color becomes too dark

<FieldManager pickerOpen={pickerOpen} setPickerOpen={setPickerOpen} />
</EuiFlexItem>
</EuiFlexGroup>
<SearchBar {...searchBarProps} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a spacer instead of a vertical flex group because IE is not able to layout them correctly

@@ -1,3 +1,14 @@
.gphVisualization {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making sure the growing flex property is propagated to the svg element. This became necessary because there is an additional wrapper element (introduced by the ngreact-directive). I wasn't able to remove the element (somehow replace: true) doesn't work for ngreact directives, thus I chose this solution. The unnecessary wrappers can be removed together with angular.


.gphGraph {
flex: 1;
overflow: hidden;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set overflow hidden to the graph for IE (other browsers do this by default).
At least this one was also released in the last version (didn't check others):
Screenshot 2019-10-09 at 11 52 16

@flash1293
Copy link
Contributor Author

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 marked this pull request as ready for review October 9, 2019 15:06
@flash1293 flash1293 requested a review from a team October 9, 2019 15:06
@flash1293 flash1293 requested a review from a team as a code owner October 9, 2019 15:06
@flash1293 flash1293 requested a review from kertal October 9, 2019 15:06
Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Looks good. Nice thing with the comments in place.

@@ -49,6 +49,10 @@
margin-bottom: 0;
}

.gphSelectionList__icon {
font-size: 0.7rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the appropriate $euiFontSizeX var here.

@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Oct 10, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@flash1293
Copy link
Contributor Author

Jenkins, test this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 merged commit d0b7e7a into elastic:master Oct 10, 2019
@flash1293 flash1293 deleted the graph/style-fixes branch October 10, 2019 13:25
flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 10, 2019
flash1293 added a commit that referenced this pull request Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Graph Graph application feature release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants