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

[WIP] improve service traffic table on mobile #1042

Merged

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Dec 14, 2016

Fixes #1031

todo

table top

service-page-tbody-loop

table bottom

service-page-traffic-table-loop-tbody-bottom

cc @spadgett

@juanvallejo juanvallejo force-pushed the jvallejo/fix-service-page-mobile branch from 5081729 to 3647e34 Compare December 14, 2016 22:31
<tr ng-repeat-start="(routeName,ports) in portsByRoute" style="display:none;"></tr>
<tr ng-repeat="port in ports" ng-if="routeName !== ''">
<tbody ng-if="(portsByRoute | hashSize) > 0" ng-repeat-start="(routeName,ports) in portsByRoute" style="display:none;"></tbody>
<tbody ng-repeat="port in ports" ng-if="routeName !== ''">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett The reason why the double border occurs at the top of the table, is because I am "nesting" two ng-repeats using sibling tbody elements. Although the first one is hidden with display: none, it is causing the second one to have a 2px border when it otherwise would not due to it normally being the first tbody element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A solution would be to add the 2px top border to tbody > tr + tr instead. That way, we could also keep the ng-repeat in the tr element, and update the pods-table directive and routes.html to do this as well.

cc @jwforres

Copy link
Member

Choose a reason for hiding this comment

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

I think we have the 2px border on other responsive tables. Can you check? If so, let's open a separate issue to fix them all.

cc @rhamilto @sg00dwin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett

I think we have the 2px border on other responsive tables. Can you check? If so, let's open a separate issue to fix them all.

It seems we do have a 2px border on the pods-table as well as the routes table but it appears to come from the table's top border rather than the tbody. In the traffic-table, the table's top border is adding 2px, and an additional 2px are getting added by the first visible tbody due to the hidden tbody right before it.

pods-table-mobile

I can go ahead and open an issue to address this by repeating the tr tag rather than the tbody, and apply this change to the pods and routes table as well.

EDIT: although the above suggestion solves repeating tbody tags, I do not believe it will solve the 4px body whenever a table deals with nested ng loops. We could have a specific rule for that table in _core.less to address this.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

How does it look if we use text-center on the arrows?

Just one small comment on the code. I agree with a follow to the fix the extra border since it's a problem with other tables.

@@ -1209,3 +1209,12 @@ copy-to-clipboard .input-group.limit-width {
width: 100%;
}
}

// swaps direction of arrow in table cell
td[role="presentation"].arrow:after {
Copy link
Member

Choose a reason for hiding this comment

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

Probably belongs in _tables.less

@spadgett spadgett self-assigned this Dec 16, 2016
@juanvallejo juanvallejo force-pushed the jvallejo/fix-service-page-mobile branch from 3647e34 to f516119 Compare December 16, 2016 14:59
@juanvallejo
Copy link
Contributor Author

@spadgett Thanks, review comment addressed.

How does it look if we use text-center on the arrows?

Because the table cell is a bit wide, they look offset from the rest of the content if I align them center.
We could add some padding to the left, but I am not sure if this would be a consistent solution for all cases.

service-page-arrows-center-mobile

@juanvallejo
Copy link
Contributor Author

Related PR: #1045

@spadgett
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to f516119

@openshift-bot
Copy link

openshift-bot commented Dec 19, 2016

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/867/) (Base Commit: a20a694)

@openshift-bot openshift-bot merged commit ef858d2 into openshift:master Dec 19, 2016
@juanvallejo juanvallejo deleted the jvallejo/fix-service-page-mobile branch December 19, 2016 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve service traffic table on mobile
3 participants