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

[bug] Network tree - node content overflows the node box if less than 35 devices #465

Closed
vladaurosh opened this issue Oct 2, 2023 · 19 comments
Labels
bug 🐛 Something isn't working next release/in dev image🚀 This is coming in the next release or was already released if the issue is Closed.

Comments

@vladaurosh
Copy link
Contributor

vladaurosh commented Oct 2, 2023

Describe the issue

Just updated from 23.7.22 to latest and there are some visual bugs.
Network diagram looks really bad.

New look:
2
3
Previous look:
1

Then trying to change device type opens blank drop down.
4

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 2, 2023

Hi there,

Can you please send me the browser you are using?

Are you zoomed in/out?

I checked on my end and it works fine on Firefox, Edge, Chrome.

Try also clearing cache and cookies.

If issues persist, can you please send me a screenshot of the inspector in your browser of one of the nodes with the css visible?

Thanks,
J

@jokob-sk jokob-sk added the Waiting for reply⏳ Waiting for the original poster to respond, or discussion in progress. label Oct 2, 2023
@vladaurosh
Copy link
Contributor Author

Hi.

Zoom in/out doesn't change anything.
I've tried both Firefox and Chrome, clearing cache, tried portable Firefox as well same issue.
Here's screenshot.
pialert

Could this be scaling issue? My laptop screen is 1366x768, don't have full hd at the moment.

@ScottRoach
Copy link

I'm having this same issue. There are two icons being listed and there is not enough room for both of them.

It looks like the problem is here: jokob-sk/Pi.Alert@7efe658#diff-e2a70fa8ded4064777a348b63e9bb6f99b064229bc37d02374ca4346ae8c4144L649-R653

I can't tell what the intended behavior is, but the markup and css are not leaving enough room.

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 3, 2023

Regrettably, I still can't reproduce the issue, even on a 1366x768 resolution. This seems to be an issue that was fixed before:

#424

Could you please verify you are on the latest release?

Here are the on-the-fly calculated values for a screen resolution of 1366x768:

image

Thanks in advance,
j

@vladaurosh
Copy link
Contributor Author

vladaurosh commented Oct 3, 2023

I've just compared my front/network.php and the one in the repo and they match.

Also in https://github.com/jokob-sk/Pi.Alert/issues/424 (previous version) boxes were not rounded as in latest version.

@vladaurosh
Copy link
Contributor Author

Just tried 1920x1080p screen,same thing. Like the box is split into 2 lines, with 2 icons in first one, and text in second one,

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 3, 2023

Hummm.
Are there any js errors in the dev console?
Can you please provide me with a similar screenshot I did containing the width /height of the netPort div on the 1366x768 resolution so we can compare like for like?

@ScottRoach
Copy link

@jokob-sk It looks like this isn't happening to you because of how many leaf nodes you have in your network diagram. In your markup the dimensions are 1.971em and in mine and @vladaurosh the dimensions are 2.7em. The emSize is calculated based on how many leaf nodes are in the diagram.

https://github.com/jokob-sk/Pi.Alert/blob/71c20e159db69a2cd6ecbc77361a8f4d1039628d/front/network.php#L613-L643

If you add the following styles to chrome you can see it how we do:

.netPort {
    width: 2.7em !important;
    height: 2.7em !important;
}
.portBckgIcon {
    margin-left: -2.5em !important;
}

It seems like any leaf node count below ~35 will present this way if I am not mistaken.

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 3, 2023

Thanks @ScottRoach!
I have 51 devices in my network diagram, how many should I try on my end to reproduce the behaviour?

@vladaurosh
Copy link
Contributor Author

I have 34 devices :)
Btw @jokob-sk, screenshot I've sent, isn't that one for "netPort"?

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 3, 2023

@vladaurosh you are right. I was confused because of the default 2.7em size. It seems like it values are not calculated as per above js code. Are there any js errors in the dev console?

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 3, 2023

@ScottRoach the issue seems to be that the quoted code is not executed on your instance, not the device count.

@ScottRoach
Copy link

@ScottRoach the issue seems to be that the quoted code is not executed on your instance, not the device count.

I'm not sure what you mean by this. I have breakpoints on the code that I referenced above and am certain that it is running.

If I manipulate the leafNodesCount variable to set it to some number > 35 before line 620 runs then it displays correctly. Here's a video of me manipulating that showing that it renders correctly with 40 and incorrectly with 6.

Screen.Recording.2023-10-03.at.2.42.13.PM.mp4

jokob-sk added a commit that referenced this issue Oct 4, 2023
@jokob-sk
Copy link
Owner

jokob-sk commented Oct 4, 2023

@ScottRoach this helped a lot, thanks!

I pushed a fix - if anyone can test the _dev image that would be great.

Thanks in advance,
j

@jokob-sk jokob-sk added bug 🐛 Something isn't working next release/in dev image🚀 This is coming in the next release or was already released if the issue is Closed. and removed Waiting for reply⏳ Waiting for the original poster to respond, or discussion in progress. labels Oct 4, 2023
@lorki97
Copy link

lorki97 commented Oct 4, 2023

Looks better than before when having only two devices:
image

The icons seem to be very close together or even overlapping, at least not with the same spacing as in jokob-sk's screenshot.

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 4, 2023

@lorki97 this is intended. I'll leave it for now and if someone wants to submit a PR I can review it and approve. I'm really not a front end expert 😅

@jokob-sk jokob-sk changed the title Visual bugs in 23.10.2 [bug] Network tree - node content overflow the node box if less than 35 devices Oct 4, 2023
@jokob-sk jokob-sk changed the title [bug] Network tree - node content overflow the node box if less than 35 devices [bug] Network tree - node content overflows the node box if less than 35 devices Oct 4, 2023
@ScottRoach
Copy link

This looks much better on the dev container. I will try to make a PR to space out the icons and center align the port number (if set). For now though this is much better!

Thanks!

@jokob-sk
Copy link
Owner

jokob-sk commented Oct 5, 2023

Thanks in advance @ScottRoach 🙏

@jokob-sk
Copy link
Owner

Released in 23.11.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working next release/in dev image🚀 This is coming in the next release or was already released if the issue is Closed.
Projects
None yet
Development

No branches or pull requests

4 participants