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

fix(placement): remove empty side placement section #11646

Closed
wants to merge 1 commit into from

Conversation

LcsGa
Copy link

@LcsGa LcsGa commented Aug 18, 2024

Summary

Fixes #11645

Problem

As explained in the issue, this PR is meant to make the sidebar-inner menu full height, as a paying user with ad-free experience.

Solution

I replaced the empty section of the SidePlacement component by null in order to render nothing.

It might not be the right solution though. The empty section was maybe there for a reason but as a fairly new contributor, I couldn't find any, thus removing this element was the best choice in that case.

If I missed something, I can rollback and simply add:

section.place {
  // ...

  &:empty {
    display: none;
  }

  //...
}

Screenshots

Before

image

After

image

@LcsGa LcsGa requested a review from a team as a code owner August 18, 2024 12:27
@LcsGa LcsGa force-pushed the feature/sidebar-inner-height branch from 68a88c7 to 7952de8 Compare August 18, 2024 12:45
@OnkarRuikar
Copy link
Contributor

OnkarRuikar commented Aug 19, 2024

MDN Plus is not available in my region. I don't know if the empty section gets populated on the client side. Also, it is not possible to test this entirely without setting up the https://github.com/mdn/rumba .

In case we resort to a CSS solution, the section.place:defined is not specific. We could update the existing selector:

-- &.side
++ &.side:not(:empty)

/ping @caugner

@caugner caugner self-assigned this Sep 10, 2024
@caugner caugner changed the title [#11645] feat(sidebar-inner): enlarge the sidebar-inner element by removing the section when it is empty fix(placement): remove empty side placement section Sep 10, 2024
@@ -64,9 +64,8 @@ export function SidePlacement() {
].filter(([_, v]) => Boolean(v))
);

return !placementData?.side ? (
<section className="place side"></section>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fiji-flo Is there a reason we add this empty section (even) if there is no placement?

@caugner
Copy link
Contributor

caugner commented Sep 10, 2024

@LcsGa Thank you for supporting MDN, and for opening the issue and this PR. ❤️

We have now fixed the issue via this PR using a slightly different approach, so I'm going ahead and close this PR.

@caugner caugner closed this Sep 10, 2024
@LcsGa
Copy link
Author

LcsGa commented Sep 10, 2024

You're welcome! The pleasure is all mine!
I checked your PR, thanks! 😁

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

Successfully merging this pull request may close these issues.

Make the sidenav-inner full height when there is no adds to display for paying users
3 participants