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 scroll to behave similar to interactors docs #865

Merged
merged 10 commits into from
Mar 5, 2024
Merged

Conversation

wKich
Copy link
Member

@wKich wKich commented Dec 18, 2023

Motivation

@cowboyd:

  • Whole document needs to scroll, not just when hovered over main content
  • Footer should not be visible when not needed
  • Footer should look like original footer before this PR Tweak CSS #857 not the “thin” footer with only one link
  • The scroll interaction should be as close to this https://frontside.com/interactors/docs as possible

Approach

  • Removed unnecessary useAppHtml.tsx
  • Fixed scroll
  • Adjust header styles
  • Tweak sidebar

@wKich wKich requested review from cowboyd and taras December 18, 2023 23:16
@cowboyd
Copy link
Member

cowboyd commented Dec 19, 2023

You're a hero!!

This is a really great first pass. It looks like the burger is a bit close to the content:

image

And then it looks like on the home page, the nav burger is getting pushed off to the right because of the badges being too wide or something.
image

@wKich
Copy link
Member Author

wKich commented Dec 19, 2023

it looks like on the home page, the nav burger is getting pushed off to the right because of the badges being too wide or something.

Nope, the burger is not a part of header content.

Moved it into the header
Screenshot 2023-12-19 at 18 59 17

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

Fantastic work. Looks really smooth and the markup is clean:

@cowboyd
Copy link
Member

cowboyd commented Dec 19, 2023

What do we think about the rounded header? It matches the interactors, but I think square might be more 2024

Thoughts?

@cowboyd cowboyd self-requested a review December 19, 2023 15:47
@wKich
Copy link
Member Author

wKich commented Dec 19, 2023

maybe only bottom corners make rounded

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

Let's just go with square for right now.

@cowboyd
Copy link
Member

cowboyd commented Dec 20, 2023

Hmmm.... I think I see what's going on. The nav burger is only on the docs route, so it doesn't work on the home page where there is no sidebar. I think we need to move it back down because it really is part of the guide.

Effection jpeg

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

It looks good, although the badges are causing an overflow which makes the page scroll left and right

Installation  Effection

@cowboyd
Copy link
Member

cowboyd commented Jan 11, 2024

@wKich What alternatives to we have to making the header and app wrappers know about what route they're rendering? I'm a bit uncomfortable making the app.html aware of what's inside it (isDocsRoute) and the header also needing to know about what is being rendered (showNav, showGuides).

Can we use a form of render props?

@wKich
Copy link
Member Author

wKich commented Jan 25, 2024

@cowboyd Could you please check it?

@cowboyd
Copy link
Member

cowboyd commented Jan 29, 2024

@wKich Having an isDocsRoute() boolean in the head just feels like it's not correct. We're missing an abstraction somewhere.

I think we have a couple of options. One would be to use a context to add navlinks.

{yield* extraNavlinks([]) }

Another, would be to use a render prop for the app html useAppHtml({ extraNavlinks: [<NavBurger/>] })

@wKich
Copy link
Member Author

wKich commented Jan 29, 2024

Oh, ok. So you mean to pass nav links into header and form them outside?

@cowboyd
Copy link
Member

cowboyd commented Jan 31, 2024

Yes. That way the container doesn't need to be aware of the content. Same way that the children are rendered into the body without needing to know what they are.

@cowboyd
Copy link
Member

cowboyd commented Feb 7, 2024

Apologies for the delay. I'm going to make sure I get this through because we're almost there.

The `Guides' link is missing on the docs page (even though it would link back to itself).

In other words, the links should not "flash" when you navigate between sections of the website as they move from one place to the other. The space should be allocated beforehand, and if you are in the mobile guides, then the burger appears.

Instead of passing the navlinks all through, made even have a "navmenu" property that will allocate that spot?

@cowboyd
Copy link
Member

cowboyd commented Feb 12, 2024

@wKich Mainly, what we want to avoid is the menu "flashing" and re-arranging when you click links.

@cowboyd
Copy link
Member

cowboyd commented Feb 13, 2024

I see what you mean about it being very squished. What about taking out the v3 from the header when it is mobile?

@taras
Copy link
Member

taras commented Feb 16, 2024

@cowboyd can we go ahead and merge this?

@cowboyd
Copy link
Member

cowboyd commented Feb 16, 2024

@cowboyd can we go ahead and merge this?

It still looks bad on mobile. I think we have to move the v3 from the header on mobile.

@taras
Copy link
Member

taras commented Feb 16, 2024 via email

@wKich
Copy link
Member Author

wKich commented Feb 29, 2024

@cowboyd Removed "v3" from the header. Sorry for delay

@cowboyd
Copy link
Member

cowboyd commented Mar 5, 2024

Ok, let's rebase and go.

wKich added 10 commits March 5, 2024 11:51
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Signed-off-by: Dmitriy Lazarev <w@kich.dev>
Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

I think we're good to go here. Squashing them into a single commit is optional as long as the commits that we have tell a coherent story.

@taras taras merged commit a860b0e into v3 Mar 5, 2024
3 checks passed
@taras taras deleted the dl/fix-docs-scroll-styles branch March 5, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants