webui: Set the device logo and cell to have a max width #5700

Merged
merged 3 commits into from Feb 14, 2017

Conversation

Projects
None yet
6 participants
@InsaneSplash
Contributor

InsaneSplash commented Jan 31, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.

  • Have you followed our code guidelines?

  • Set a maximum width for the device logo.

  • Set a maximum width for the cell that the device image is displayed in.

This keeps the device name text in the same place where in the past it would be moving all over the place.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jan 31, 2017

Thank you for submitting a PR @InsaneSplash! We have found the following @paulgear, @laf and @florianbeer based on the history of these files to review this PR.

Thank you for submitting a PR @InsaneSplash! We have found the following @paulgear, @laf and @florianbeer based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Jan 31, 2017

Member

I didn't think setting column width was good from a fluid perspective? However this just adds a lot of white space next to icons not 220 wide.

Imho looks better as is.

Member

laf commented Jan 31, 2017

I didn't think setting column width was good from a fluid perspective? However this just adds a lot of white space next to icons not 220 wide.

Imho looks better as is.

@InsaneSplash

This comment has been minimized.

Show comment
Hide comment
@InsaneSplash

InsaneSplash Feb 2, 2017

Contributor

I agree with your points, the problem is 2 fold.

  1. The icon presented in the title bar changes in size depending on the width. For example CentOS/Fedora being a more square icon is one size, but when using a logo like Radwin/Lenovo it becomes a massive picture so the sizing needs to be kept in check no matter the original size of the icon.

  2. The problem with the way the title bar is built with the graphs, is that as the page loads and the graphs start to be included, the title moves around in the title bar. This fixed cell for the picture stops the title moving around as the browser loads and will keep it aligned to the left next to a cell that does not change its width.

  3. This also keeps the title in the same place in the title no matter the icon size.

Examples:
Before:
capture-mt

capture-radwin

capture-lenovo

After:
image

image

image

Contributor

InsaneSplash commented Feb 2, 2017

I agree with your points, the problem is 2 fold.

  1. The icon presented in the title bar changes in size depending on the width. For example CentOS/Fedora being a more square icon is one size, but when using a logo like Radwin/Lenovo it becomes a massive picture so the sizing needs to be kept in check no matter the original size of the icon.

  2. The problem with the way the title bar is built with the graphs, is that as the page loads and the graphs start to be included, the title moves around in the title bar. This fixed cell for the picture stops the title moving around as the browser loads and will keep it aligned to the left next to a cell that does not change its width.

  3. This also keeps the title in the same place in the title no matter the icon size.

Examples:
Before:
capture-mt

capture-radwin

capture-lenovo

After:
image

image

image

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 2, 2017

Member

Can we do it without the td width and just rely on css and do min-width?

Member

laf commented Feb 2, 2017

Can we do it without the td width and just rely on css and do min-width?

@InsaneSplash

This comment has been minimized.

Show comment
Hide comment
@InsaneSplash

InsaneSplash Feb 3, 2017

Contributor

I'll try move this in to the CSS :)

Contributor

InsaneSplash commented Feb 3, 2017

I'll try move this in to the CSS :)

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

@InsaneSplash InsaneSplash changed the title from Set the device logo to have a max width and set the td cell to also h… to Set the device logo and cell to have a max width Feb 3, 2017

@InsaneSplash InsaneSplash changed the title from Set the device logo and cell to have a max width to webui: Set the device logo and cell to have a max width Feb 3, 2017

@laf

laf approved these changes Feb 3, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Feb 3, 2017

Member

I'm not sure what the goal of the colgroup is , but it forces the graphs onto a second line at certain sizes.

Member

murrant commented Feb 3, 2017

I'm not sure what the goal of the colgroup is , but it forces the graphs onto a second line at certain sizes.

@InsaneSplash

This comment has been minimized.

Show comment
Hide comment
@InsaneSplash

InsaneSplash Feb 4, 2017

Contributor

Would you have an example? I've tested against different resolutions and haven't seen that. I didn't set any sizes for the remaining td cells.

Contributor

InsaneSplash commented Feb 4, 2017

Would you have an example? I've tested against different resolutions and haven't seen that. I didn't set any sizes for the remaining td cells.

@InsaneSplash

This comment has been minimized.

Show comment
Hide comment
@InsaneSplash

InsaneSplash Feb 4, 2017

Contributor

The colgroup in HTML5 helps to set the attributes for each cell in the table. The problem is there is a need to set both the image max width in line with also setting the cell's max width. Ultimately I think the title bar table should has a set width for each field to stop the browser from working it out as the data and graphs are loaded.

I am going to try change it a little bit to work with %'s and include all 3 cells. Its not an easy one to solve, but I feel its important to sort it out as from the included pics, its just weird looking :)

Contributor

InsaneSplash commented Feb 4, 2017

The colgroup in HTML5 helps to set the attributes for each cell in the table. The problem is there is a need to set both the image max width in line with also setting the cell's max width. Ultimately I think the title bar table should has a set width for each field to stop the browser from working it out as the data and graphs are loaded.

I am going to try change it a little bit to work with %'s and include all 3 cells. Its not an easy one to solve, but I feel its important to sort it out as from the included pics, its just weird looking :)

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Feb 4, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Feb 4, 2017

Member

Have you tried switching it to bootstrap?

Member

murrant commented Feb 4, 2017

Have you tried switching it to bootstrap?

@InsaneSplash

This comment has been minimized.

Show comment
Hide comment
@InsaneSplash

InsaneSplash Feb 5, 2017

Contributor

I think we would need to remove the table, and go with a grid to achieve the bootstrap conversion?

Contributor

InsaneSplash commented Feb 5, 2017

I think we would need to remove the table, and go with a grid to achieve the bootstrap conversion?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Feb 6, 2017

Member

Correct, I didn't check how much work it would be to do so. Just asking.

Member

murrant commented Feb 6, 2017

Correct, I didn't check how much work it would be to do so. Just asking.

+
+.device-title-bar-1 {
+ width: 25%;
+ max-width: 25%;

This comment has been minimized.

@murrant

murrant Feb 8, 2017

Member

Isn't max-width redundant when you already set width to a percent?

@murrant

murrant Feb 8, 2017

Member

Isn't max-width redundant when you already set width to a percent?

This comment has been minimized.

@InsaneSplash

InsaneSplash Feb 8, 2017

Contributor

I read up somewhere that its advisable to include it for "older, non compliant browsers" so I put in there for safety. It didn't make any difference to me, but wanted to try cover all bases. Want me to remove it?

@InsaneSplash

InsaneSplash Feb 8, 2017

Contributor

I read up somewhere that its advisable to include it for "older, non compliant browsers" so I put in there for safety. It didn't make any difference to me, but wanted to try cover all bases. Want me to remove it?

This comment has been minimized.

@murrant

murrant Feb 9, 2017

Member

No, idea I'm not a web expert by any means. I was mostly genuinely curious.

@murrant

murrant Feb 9, 2017

Member

No, idea I'm not a web expert by any means. I was mostly genuinely curious.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 9, 2017

Member

I'm ok with this now if someone else is?

Member

laf commented Feb 9, 2017

I'm ok with this now if someone else is?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Feb 12, 2017

Member

trigger ci

Member

laf commented Feb 12, 2017

trigger ci

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment

@laf laf merged commit 3843346 into librenms:master Feb 14, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment