Issue #16243: Fixed margin-left of #bodyColumn #16251
Conversation
| #bodyColumn { | ||
| margin-left: 286px; | ||
| } |
There was a problem hiding this comment.
we dont need this anymore as we're calculating margin-left in JS
|
github, generate site |
| @@ -80,6 +80,9 @@ function setBodyColumnMargin() { | |||
| if (window.innerWidth < 823 && !document.querySelector("#hamburger")) { | |||
| setCollapsableMenuButton(); | |||
| } | |||
|
|
|||
| const leftColumnWidth = leftColumn.offsetWidth; | |||
| bodyColumn.style.marginLeft = `${leftColumnWidth + 15}px`; | |||
There was a problem hiding this comment.
Fixing style layout problems by js is not ok.
Please share details on why it is not possible to be css only fix.
There was a problem hiding this comment.
If you visit some page where sidebar is small, you won't see the problem, for example: https://checkstyle.org/index.html
But in some pages the sidebar becomes very large, for example: https://checkstyle.org/checks/naming/abbreviationaswordinname.html#AbbreviationAsWordInName
Here you can see that there is no space between sidebar and main content. It is happening because I removed following JS in one of my previous PR as I thought we don't need it but I was wrong: https://github.com/checkstyle/checkstyle/pull/16239/files#diff-74191770f323d7fd9586fae43dcc6310dec524a85cc7fcfaa03e69a2cf089eeaL90-L91
We cannot fix this problem only by using CSS because sidebar's width changes dynamically based on which page we're visiting. In the above example, the AbbreviationAsWordInName name is too long so sidebar's width also becomes too large. We cannot dynamically define sidebar's width with CSS only, for that we require JS.
Currently, we have defined static width of the sidebar in site.css but I have removed it in this PR as mentioned here: https://github.com/checkstyle/checkstyle/pull/16251/files#r1931496896
I have added back the JS I removed in one of my previous PR which calculates the width of the sidebar dynamically and adds spacing between sidebar and main content of the page.
There was a problem hiding this comment.
dynamic width is only for mobile mode, as far as I understand.
long text can wrap in width of area it has, having two lines for long name is in menu is ok.
is it possible ?
There was a problem hiding this comment.
long text can wrap in width of area it has, having two lines for long name is in menu is ok.
wrapping text in menu will confuse users, it will become hard for them to read the Check name. IMO we should not wrap text
There was a problem hiding this comment.
dynamic width is only for mobile mode, as far as I understand.
the dynamic width of sidebar is not our concern. The problem is placement of main content of the page, it statically gets it margin-left from site.css.
This was SS of current master branch site, the blue part the of main content and orange part is margin-left of main content of body, you can see the blue part is overlapping sidebar. So if you try to click something on that overlapped area of sidebar, it won't work.
Why? because we gave static margin-left to our main content in site.css, and also there seem to be some placement related issue, Im not able to figure out that placement issue. But it's not good to give static margin from our site.css, instead we should rely on JS for that.
There was a problem hiding this comment.
ok, this explains why we should remove strict width #bodyColumn {.
So it(left panel) should take now all horizontal space that it needs, so there should be some way to configure it by CSS.
function setBodyColumnMargin() { should exists only for mobile mode of website, not for big screens.
@SheikhZaeem , may be you know a CSS trick to make it possible?
There was a problem hiding this comment.
ok, this explains why we should remove strict width
Yes, this will also require to revert the code which made the sidebar to have fixed width.
In normal cases this is possible by only using CSS but we're using maven-site-plugin so site has some predefined structure so making the sidebar to have dynamic width with only using CSS might be very tricky or even impossible... Not sure about this
There was a problem hiding this comment.
Dynamic width is only in mobile mode, and there we have wrapping of long names, so no problems with dynamic width.
We have issue for case for desktop rendering , when all is well defined. We can disable hamburger, and it works same, with overlap.
|
Github, generate site |
Zopsss
force-pushed
the
16243-bodyColumn-margin
branch
from
d19ae2b to
66405f1
Compare
|
I like this wrapping of text, can we do same in non mobile mode? |
|
it is possible but it'll make it hard to read long check names so I think we should stick with dynamic width
|
|
How was it in the old site? |
|
it wasn't wrapping:
|
|
@mahfouz72 , do you like wrap of long names or how it is now - overlap ? actually fixed at #16297 (review) please approve if you like it. This PR is not conflicting to that PR, we can merge both. |
@romani I don't like either of them, Overlapping is bad for UI, and wrapping long names makes them hard to read and inconvenient. |
|
My concern here is usage of JS for layout, it smells very badly, usually red flag. |
JS is not that heavy, it's only of two lines. Currently it is inside This is a very small JS code so it'll not affect user's experience even if we keep it as it is now. |
All mess in engineering is starting from this phrase :) . |
|
GitHub, generate website |
#16243
Fixed: #16243 (comment)