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

blank space (not a taylor swift song) #1855

Open
Qfrijters opened this issue Oct 19, 2023 · 8 comments · May be fixed by #2115
Open

blank space (not a taylor swift song) #1855

Qfrijters opened this issue Oct 19, 2023 · 8 comments · May be fixed by #2115
Labels
bug Something isn't working Frontend Issue or PR related to the frontend code
Projects

Comments

@Qfrijters
Copy link

This is maybe not a big feature request, but I just noticed there are massive blank spaces between some sections.
Take Ingredients and nutrition information as example. This is at 100% in MS Edge. It might be just me being annoying, that's true, was just wondering if you could bring everything a bit closer together

msedge_y1tymkvYAE.mp4
@Qfrijters Qfrijters added the enhancement New feature or request label Oct 19, 2023
@christianlupus
Copy link
Collaborator

May I ask what resolution you were using? Then I might be able to reproduce...

@Qfrijters
Copy link
Author

Qfrijters commented Oct 19, 2023

Uploading msedge_yZEyIrDDzU.mp4…

i'm in 2560x1440, that's my full window
the actual black part is 2504x1385 (give or take a couple pixels)
image

@Qfrijters
Copy link
Author

msedge_yZEyIrDDzU.mp4

@christianlupus christianlupus added Frontend Issue or PR related to the frontend code bug Something isn't working and removed enhancement New feature or request labels Oct 29, 2023
@christianlupus christianlupus added this to To do in UI rework via automation Oct 31, 2023
@j0hannesr0th
Copy link
Contributor

Hi @christianlupus can I work on this? I have this bug on all my devices (Desktop, Tablet and Smartphone). I wrote this bug on some other ticket as a side note, but don't know which one it was.

@christianlupus
Copy link
Collaborator

I remember you had the idea of getting rid of the grid structure of I am right. Unfortunately, on mobile, I cannot find that comment quickly.

For sure, you can try to get this fixed. I would suggest to do it in a minimal CSS-only solution that does not restructure everything. There are

  1. bigger changes on the agenda and
  2. I have to prepare the NC28 release very soon.

For number 1 I suggest not to invest to much into this topic as it might be reworked anyways. Number 2 means that you might need to rebase later on but this is okay I think.

I suggest not to remove the grid as this would cause other issues with mobile and tablet view. I have not found a way to change the structuring of the page in a similar manner. When in doubt, just wow and we might chat or have a short call if that helps.

Keep me posted please.

@philiprenich
Copy link
Contributor

Here's the line of CSS causing the layout problem.

grid-template-rows: 100% 100% 100% 1fr;

When a percentage is used for this property, it is calculated based on the grid container. So the first 3 columns are 100% of the grid container. To figure out the grid container size, everything is calculated as if the rows were auto and that value is then used for 100%. That means rows with less than others are still big.

Oddly, Firefox doesn't do this and I think that's actually a difference/bug in their interpretation of the spec (as I understand it at least).

IMO, that CSS rule should just be removed and the rows should be sized implicitly, that is, auto.


Additionally, I think the line above grid-template-columns: 1fr 1em 2fr; should be changed to grid-template-columns: 1fr 2fr; with an added gap: 1em; rule or gap-column: 1em instead. (I wonder if the grid code was written prior to grid getting gap?)

There's a couple other little CSS oddities I'm noticing, but they are directly related to this so I'll leave them out of this.

@christianlupus @j0hannesr0th do you want me to create a PR for this? Would love to see it solved to make the app much more usable on mobile! Cheers!

@j0hannesr0th j0hannesr0th removed their assignment Apr 8, 2024
@j0hannesr0th
Copy link
Contributor

Hi, @philiprenich yes, create a PR.

@philiprenich
Copy link
Contributor

Done. Though (and apologies in advance) I don't yet have an NC dev environment to test code. This is the first I've looked to contribute. It's a small CSS change so pretty safe, but if you want to hold off on it until I can run it through a real installation and not Dev Tools, that's fine - and I don't want to create more work for maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Frontend Issue or PR related to the frontend code
Projects
Development

Successfully merging a pull request may close this issue.

4 participants