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

Apparent layout change on small screens since 1.14.0 #1215

Closed
colinl opened this issue Aug 17, 2024 · 22 comments · Fixed by #1218, #1223 or #1234
Closed

Apparent layout change on small screens since 1.14.0 #1215

colinl opened this issue Aug 17, 2024 · 22 comments · Fixed by #1218, #1223 or #1234
Assignees
Labels
bug Something isn't working size:XS - 1 Sizing estimation point

Comments

@colinl
Copy link
Contributor

colinl commented Aug 17, 2024

Current Behavior

I am not sure if this is a regression or an intended change. I have a page with Layout Fixed and Theme Sizings settings:

image

Using version 1.14.0 of the dashboard, in a small window, and on a tablet, with three groups each 3 wide, it looks like

Screenshot from 2024-08-17 08-48-51

but with the current main branch (16th Aug 2024), in the same window size, it only shows two groups across, pushing the third off the screen.

image

Is this an intended change, and if it is how do I restore the previous functionality please?

Expected Behavior

No response

Steps To Reproduce

No response

Environment

  • Dashboard version: Current main branch, commit cd5d1ce
  • Node-RED version: 4.0.2
  • Node.js version:
  • npm version:
  • Platform/OS:
  • Browser:

Have you provided an initial effort estimate for this issue?

I am not a FlowFuse team member

@colinl colinl added bug Something isn't working needs-triage Needs looking at to decide what to do labels Aug 17, 2024
@joepavitt
Copy link
Collaborator

Hmmm, certainly not intentional - the padding/sizings look consistent, it appears the gauges themselves are rendering wider?

@colinl
Copy link
Contributor Author

colinl commented Aug 20, 2024

Running two instances of node-red, both with only the dashboard installed, one with 1.14.0 and one with main from github, both running only the flow below, which is three text widgets, each 3x1, in three groups of size 3x1, in a PC I see

image

That is both running at the same time with a page open on each in Edge browser. It can be seen that the latest main has significantly larger groups than 1.14.0, which means they do not fit on the tablet. There are no CSS overrides, only those three nodes and default theme.

I notice that the colour of the text has changed too, was that intentional?

[{"id":"cdf515413f178f83","type":"ui-text","z":"bb54aef01db48493","group":"6255fc3f7023c25f","order":1,"width":"3","height":"1","name":"text 1","label":"text 1","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":470,"y":260,"wires":[]},{"id":"78c35eff33a58ee3","type":"ui-text","z":"bb54aef01db48493","group":"4d52b7074513b4cf","order":1,"width":"3","height":"1","name":"text 2","label":"text 2","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":470,"y":320,"wires":[]},{"id":"a7be58d828009851","type":"ui-text","z":"bb54aef01db48493","group":"902105b55c678f0e","order":1,"width":"3","height":"1","name":"text 3","label":"text 3","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":470,"y":380,"wires":[]},{"id":"6255fc3f7023c25f","type":"ui-group","name":"Three groups test 1","page":"e499766061486e72","width":"3","height":"1","order":1,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"4d52b7074513b4cf","type":"ui-group","name":"Three groups test 2","page":"e499766061486e72","width":"3","height":"1","order":2,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"902105b55c678f0e","type":"ui-group","name":"Three groups test 3","page":"e499766061486e72","width":"3","height":"1","order":3,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"e499766061486e72","type":"ui-page","name":"Three groups test 1.14.0","ui":"ID-BASE-1","path":"/page11","icon":"home","layout":"flex","theme":"a965ccfef139317a","order":1,"className":"","visible":"true","disabled":"false"},{"id":"ID-BASE-1","type":"ui-base","name":"Dashboard","path":"/dashboard","includeClientData":true,"acceptsClientConfig":["ui-control","ui-notification","ui-gauge-classic"]},{"id":"a965ccfef139317a","type":"ui-theme","name":"Default","colors":{"surface":"#404040","primary":"#109fbc","bgPage":"#e8e8e8","groupBg":"#d6d6d6","groupOutline":"#6fbc10"},"sizes":{"pagePadding":"12px","groupGap":"12px","groupBorderRadius":"4px","widgetGap":"12px"}}]

@joepavitt
Copy link
Collaborator

I notice that the colour of the text has changed too, was that intentional?

Not that I can recall - this will need some investigating

@joepavitt
Copy link
Collaborator

joepavitt commented Aug 20, 2024

PR opened to cover the text colour, the other piece is actually a feature change in: #1166

The theme you have should be switched to a "Row Height" of "Comortable (36px)" (default is 48px). We had some inconsistencies between Grid/Fixed layouts that were addressed there, and the "Comfortable" layout will give you what you had before for Fixed layouts.

@joepavitt joepavitt moved this from Backlog to Under Review in Dashboard Backlog Aug 20, 2024
@joepavitt joepavitt self-assigned this Aug 20, 2024
@joepavitt joepavitt added size:XS - 1 Sizing estimation point and removed needs-triage Needs looking at to decide what to do labels Aug 20, 2024
@colinl
Copy link
Contributor Author

colinl commented Aug 20, 2024

Unfortunately changing the row height to Comfortable did not appear to change anything. I restarted node-red and cleared the browser cache. Did you check it with my flow?

@joepavitt
Copy link
Collaborator

Yes, otherwise I wouldn't have recommended it 😁

How many themes do you have locally, are you sure you're changing the correct one?

@joepavitt
Copy link
Collaborator

Default

Screenshot 2024-08-20 at 14 04 05

Comfortable

Screenshot 2024-08-20 at 14 04 30

Compact:

Screenshot 2024-08-20 at 14 02 46

@colinl
Copy link
Contributor Author

colinl commented Aug 20, 2024

For both of them I created new user directories and then ran node red for that directory. Then I installed just the dashboard and configured the text nodes.

Did you try it on 1.14.0? The differences between the image you posted is not as great as I see between the two versions.

@joepavitt
Copy link
Collaborator

So the height is working as I'd expect, but it does look like we have an issue with the card's width in Fixed views. No matter what I change the group width to, they're always rendering at 320px wide - which accordingly to the git history, is a line of code I put in 14 months ago - so need to dig into what's changed 🤔

@github-project-automation github-project-automation bot moved this from Under Review to Done in Dashboard Backlog Aug 21, 2024
@colinl
Copy link
Contributor Author

colinl commented Aug 21, 2024

I don't think this should have been closed by #1218, there are still unresolved issues.

@joepavitt
Copy link
Collaborator

Correct - looks like GH auto-closed it because I'd mentioned it

@joepavitt joepavitt reopened this Aug 21, 2024
@colinl colinl mentioned this issue Aug 21, 2024
10 tasks
@joepavitt
Copy link
Collaborator

joepavitt commented Aug 21, 2024

Just tracking this back, the changes came in here: https://github.com/FlowFuse/node-red-dashboard/pull/1166/files#r1725334033

We had an inconsistency that 1 unit of size in Grid/Notebook/Tabs was 48px but in Fixed it was 45px - this change made them consistent (and configurable via the different layout types), but as a result, meant that the cells are now wider in "Fixed"

The solution here will be to change the width of the elements/groups to be slightly narrower.

@joepavitt
Copy link
Collaborator

Whilst the above is true, there is still an underlying bug introduced whereby the group width is not taken into account at all when rendering the groups. Have the fix, not sure how it's changed though, as the line I need to remove is from 14 months ago

@colinl
Copy link
Contributor Author

colinl commented Aug 22, 2024

I still don't think this is right. This flow has three groups of width 3. The first has three text nodes of size 1x1 and the other groups the text nodes are 3x1. In 1.14.0 this looks like this, which is correct I think

image

In the latest it looks like

image

[{"id":"dcdf6d0bcedf02e5","type":"ui-text","z":"371f5f719cb5e2ab","group":"a5e220219816243f","order":1,"width":"1","height":"1","name":"","label":"text_1.1","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":140,"y":160,"wires":[]},{"id":"5b24e7fe449f6810","type":"ui-text","z":"371f5f719cb5e2ab","group":"a5e220219816243f","order":3,"width":"1","height":"1","name":"","label":"text_1.3","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":460,"y":160,"wires":[]},{"id":"40af9b6e24de780e","type":"ui-text","z":"371f5f719cb5e2ab","group":"a5e220219816243f","order":2,"width":"1","height":"1","name":"","label":"text_1.2","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":300,"y":160,"wires":[]},{"id":"cbf83482e17c2b4b","type":"ui-text","z":"371f5f719cb5e2ab","group":"e0f34190373ab7df","order":1,"width":"3","height":"1","name":"","label":"text_2.1","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":720,"y":160,"wires":[]},{"id":"09307acb332b44cf","type":"ui-text","z":"371f5f719cb5e2ab","group":"e0f34190373ab7df","order":3,"width":"3","height":"1","name":"","label":"text_3.3","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":720,"y":280,"wires":[]},{"id":"ed10149bb6492858","type":"ui-text","z":"371f5f719cb5e2ab","group":"e0f34190373ab7df","order":2,"width":"3","height":"1","name":"","label":"text_2.2","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":720,"y":220,"wires":[]},{"id":"b50c4c40032edab9","type":"ui-text","z":"371f5f719cb5e2ab","group":"f65b9a76255edbd0","order":1,"width":"3","height":"1","name":"","label":"text_3.1","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":940,"y":160,"wires":[]},{"id":"60cc180687fb0928","type":"ui-text","z":"371f5f719cb5e2ab","group":"f65b9a76255edbd0","order":3,"width":"3","height":"1","name":"","label":"text_3.3","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":940,"y":280,"wires":[]},{"id":"c218ab2fed9e6fce","type":"ui-text","z":"371f5f719cb5e2ab","group":"f65b9a76255edbd0","order":2,"width":"3","height":"1","name":"","label":"text_2.2","format":"{{msg.payload}}","layout":"row-spread","style":false,"font":"","fontSize":16,"color":"#717171","className":"","x":940,"y":220,"wires":[]},{"id":"a5e220219816243f","type":"ui-group","name":"Group1","page":"4527544ddbb29343","width":"3","height":"1","order":1,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"e0f34190373ab7df","type":"ui-group","name":"Group 2","page":"4527544ddbb29343","width":"3","height":"1","order":2,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"f65b9a76255edbd0","type":"ui-group","name":"Group 3","page":"4527544ddbb29343","width":"3","height":"1","order":3,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"4527544ddbb29343","type":"ui-page","name":"1.14.0","ui":"4df7e5236771d85d","path":"/page1","icon":"home","layout":"flex","theme":"30b58d6104e752ec","order":1,"className":"","visible":"true","disabled":"false"},{"id":"4df7e5236771d85d","type":"ui-base","name":"My Dashboard","path":"/dashboard","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control"],"showPathInSidebar":false,"showPageTitle":true,"navigationStyle":"default","titleBarStyle":"default"},{"id":"30b58d6104e752ec","type":"ui-theme","name":"Default Theme","colors":{"surface":"#ffffff","primary":"#0094CE","bgPage":"#eeeeee","groupBg":"#ffffff","groupOutline":"#cccccc"},"sizes":{"pagePadding":"12px","groupGap":"12px","groupBorderRadius":"4px","widgetGap":"12px"}}]

@joepavitt
Copy link
Collaborator

In 1.14.0 and below we actually had a bug where all groups were a fixed size, no matter what group size you set, which is what you've seen. I did fix that, but it appears there may be more at play:

Screenshot 2024-08-22 at 10 15 20 Screenshot 2024-08-22 at 10 15 32

@gayanSandamal can you investigate this as a priority please?

@joepavitt joepavitt reopened this Aug 22, 2024
@joepavitt joepavitt moved this from Done to In Progress in Dashboard Backlog Aug 22, 2024
@colinl
Copy link
Contributor Author

colinl commented Aug 22, 2024

It seems to be OK in Grid mode, it is just Fixed where I see the problem.

@colinl
Copy link
Contributor Author

colinl commented Aug 22, 2024

In 1.14.0 and below we actually had a bug where all groups were a fixed size, no matter what group size you set

In 1.14.0 if I set group three to 6 wide I see this, which appears to contradict the suggestion that they were all the same size, unless I misunderstand what you mean.

image

@joepavitt
Copy link
Collaborator

In which case, something changed between 1.14.0 and the main branch I was testing last night to force them all to the same size instead then 🤔

Thanks for the investigation - I've asked @gayanSandamal to take a look at this

@gayanSandamal
Copy link
Contributor

In which case, something changed between 1.14.0 and the main branch I was testing last night to force them all to the same size instead then 🤔

Thanks for the investigation - I've asked @gayanSandamal to take a look at this

@joepavitt I approved this PR because the expected behavior was there

#1223 (review)

@joepavitt
Copy link
Collaborator

I know, but as Colin has pointed out, and I can see with his shared flow, something is still broken as those groups should all be the same size, and tou can see in my shared screenshot that the unit sizing is inconsistent

@gayanSandamal
Copy link
Contributor

In 1.14.0 and below we actually had a bug where all groups were a fixed size, no matter what group size you set, which is what you've seen. I did fix that, but it appears there may be more at play:

Screenshot 2024-08-22 at 10 15 20 Screenshot 2024-08-22 at 10 15 32
@gayanSandamal can you investigate this as a priority please?

@joepavitt Sorry, I didn't see this message as I was checking the thread on my phone. I'm currently looking into this with the flow given by @colinl #1215 (comment)

@colinl
Copy link
Contributor Author

colinl commented Aug 26, 2024

Great. Looking good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment