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

nav: unnecessary gaps in sidebar #724

Closed
algomaster99 opened this issue Oct 22, 2019 · 15 comments · Fixed by #991
Closed

nav: unnecessary gaps in sidebar #724

algomaster99 opened this issue Oct 22, 2019 · 15 comments · Fixed by #991
Labels
p1-important Active priorities to deal within next sprints 🐛 type: bug Something isn't working. website: eng-doc DEPRECATED JS engine for /doc

Comments

@algomaster99
Copy link
Contributor

algomaster99 commented Oct 22, 2019

I think there is unnecessary space between 'Shared Development Server' and 'User Guide'.
Screenshot from 2019-10-22 00-05-06

UPDATE: Note that a PR for this could also solve this related issue: #856

@shcheklein
Copy link
Member

@algomaster99 I can's reproduce this. What browser do you use? Is it production?

@shcheklein shcheklein added 🐛 type: bug Something isn't working. A: docs Area: user documentation (gatsby-theme-iterative) p1-important Active priorities to deal within next sprints website: eng-doc DEPRECATED JS engine for /doc labels Oct 22, 2019
@jorgeorpinel
Copy link
Contributor

I also don't see this. Try ford reloading the site on your browser Aman. Shall we close as invalid?

@algomaster99
Copy link
Contributor Author

algomaster99 commented Oct 22, 2019

@shcheklein @jorgeorpinel I sometimes see this and sometimes not. I can't figure out what is happening.
Btw, I use Chrome Version 77.0.3865.120

Here is a screenshot in production.
Screenshot from 2019-10-23 02-15-34

@shcheklein
Copy link
Member

@algomaster99 could you check the console? could you check the inspector in this place?

@algomaster99
Copy link
Contributor Author

@shcheklein
Screenshot from 2019-10-23 02-20-14

The harcoded height: 119px might be a problem. I will have too see.

@jorgeorpinel
Copy link
Contributor

I get 94px correctly. But I was able to reproduce from https://www.webpagetest.org/result/191022_SR_d9e1db4a5e961674a3f0fd9d512cd22a/ so it's definitely not just Aman's browser.

@shcheklein
Copy link
Member

@algomaster99 @algomaster99 does it happen with other sections?

@algomaster99
Copy link
Contributor Author

@shcheklein No, I don't think so.

@shcheklein shcheklein changed the title engine: uneccessay newline in the sidebar engine: uneccessary newline in the sidebar Oct 23, 2019
@shcheklein
Copy link
Member

Got the same problem with this configuration:

Screen Shot 2019-10-23 at 11 56 40 AM

Probably related to the way item height is calculated when it's close to the very edge and close to taking two lines

@iAdramelk
Copy link
Contributor

I think I found out why this bug happens:

  1. We use custom font Brandon on our site, but before font file is loaded other font is shown (system default sans-serif font).
  2. And sans-serif is wider then the Brandon. Because of that some menu items are wrapped to the second line.
  3. So if the script that calculates block height for animations is fired before font was loaded it calculates incorrect height which results in the bug.

There is a few possible solutions for this problem:

  1. Add white-space: nowrap to links, it will prevent the bug, but we will not be able to have multiline links anymore.
  2. Make menu column wider to prevent line number from changes.
  3. Start listening to the font loading event before measuring block height (but sections will stay closed for some time before font loaded).
  4. Remove animations from opening and closing submenus.
  5. Replace css-animations there with some library that will do it in js. (Will make our bundle larger)

I'd prefer first solution, but like to discuss it with everyone before implementing.

@shcheklein
Copy link
Member

Thanks, @iAdramelk - good investigation!

Let me clarify a few things re the proposed solutions:

Add white-space: nowrap to links, it will prevent the bug, but we will not be able to have multiline links anymore.

No multiline items anymore, right? How would we render those that do not fit? Can we simplify the code besides white-space: nowrap (height calculate, etc)

Make menu column wider to prevent line number from changes.

Not sure how would it solve it. Could you elaborate?

3-4-5

All of them sound hacky and/or suboptimal to me.

Agreed on the first one if understand it right ... but we should properly cut the text with ... and show the full name when you hover, for example, if it's too wide to fit into the column and simplify all the logic around multiline support.

@jorgeorpinel
Copy link
Contributor

  1. Add white-space: nowrap to links...

No multiline items anymore, right? How would we render those that do not fit?

So this has already been implemented guys? I see long items are truncated (without "..." or hover text) into a single line now e.g.:

image

image

  1. Start listening to the font loading event before measuring block height...

I also like this one. I don't see it being hacky. Could display a "loading navigation" thingy meanwhile in case it lags noticeably but do we really expect this lag to be noticeable? Maybe just the very first time a person loads it after deploying updates to prod.

@iAdramelk
Copy link
Contributor

No, I didn't have time to implement in so far. Text still should still wrap lines right now. If it didn't it is possible that it happens because of the same bug with the wrong height.

@jorgeorpinel jorgeorpinel changed the title engine: uneccessary newline in the sidebar engine: unnecessary gaps in the sidebar Dec 12, 2019
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 12, 2019

Ah yes, they do wrap. I was editing the last item in an expanded section, and the next line is hidden after it wraps. This is how the previous item looks when elongated:

image

(The next item, Community Tutorials, becomes almost comlpetely hidden as it goes over the height of the Tutorials container div.)

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 27, 2019

From #882 (comment)

Please double check the error Warning: NaN is an invalid value for theheight css style property. from line 29 in

for (let i = 0; i < reversePaths.length; i++) {
const current = reversePaths[i]
height += heightMap[current]

is no longer thrown in the new implementation.

@jorgeorpinel jorgeorpinel changed the title engine: unnecessary gaps in the sidebar nav: unnecessary gaps in sidebar Dec 27, 2019
@jorgeorpinel jorgeorpinel removed the A: docs Area: user documentation (gatsby-theme-iterative) label Jan 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-important Active priorities to deal within next sprints 🐛 type: bug Something isn't working. website: eng-doc DEPRECATED JS engine for /doc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants