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

Flyout: show language in header #360

Merged
merged 9 commits into from
Oct 10, 2024
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Aug 8, 2024

This is a simple an quick implementation of #359 just as a way to share an idea. We can discuss here if this is the path we want to follow or not. I'm open for changes.

Screenshot_2024-08-08_12-57-27

Closes #359

This is a simple an quick implementation of #359 just as a way to share an idea.
We can discuss here if this is the path we want to follow or not. I'm open for changes.

Closes #359
@humitos humitos requested a review from a team as a code owner August 8, 2024 10:59
@humitos humitos requested a review from agjohnson August 8, 2024 10:59
@nijel
Copy link

nijel commented Aug 8, 2024

Looks good to me, this is undoubtedly what I hoped for!

@orangesunny
Copy link

Thank you, @humitos!

@agjohnson
Copy link
Contributor

agjohnson commented Aug 13, 2024

For a second version of our flyout, I'd like to see the version and language in the flyout collapsed view, and probably would like to see our logo reduced or removed. The logo takes up a lot of space and doesn't exactly communicate "click here for hosted features".

With all three of these though, we'll need to have a better plan on how to handle long version names on the collapsed view. Right now, we already have display issues when the version is long (and/or can't wrap):

image

This will be worse with the language icon and text. It's also why I felt we should be careful with replacing text with icons.

@humitos
Copy link
Member Author

humitos commented Aug 14, 2024

Yeah, I think we will need to re-design the flyout at some point (maybe as part of #53 implementing gradual changes).

I'm not worried about those versions with long names for now, they seem like an edge case and it's fine if we don't support them for now.

Having the language on the collapsed version of the flyout is a great improvement and I'd like to move forward with this proposal for now.

@orangesunny
Copy link

For a second version of our flyout, I'd like to see the version and language in the flyout collapsed view, and probably would like to see our logo reduced or removed. The logo takes up a lot of space and doesn't exactly communicate "click here for hosted features".

I agree that the logo would benefit from some change. The current state looks more like an advertisement for RTD than a feature of the opened project’s docs; this disturbs users' focus.

@humitos
Copy link
Member Author

humitos commented Aug 14, 2024

I opened #364 to continue the conversation about reducing/removing the logo and make it obvious there are features inside the flyout.

@humitos
Copy link
Member Author

humitos commented Aug 14, 2024

I updated this PR to fix the tests and added the missing CSS that I didn't push the other day. I'm happy to merge this PR with these changes since I think it's an improved version of the current flyout.

@agjohnson
Copy link
Contributor

My point about long version names is that wrapping already happens. With the addition of a language element, even short version names will cause this wrapping, this isn't an issue with just long version names. This all does make the flyout feel a bit half baked, we should address issues with wrapping before adding more content.

@humitos
Copy link
Member Author

humitos commented Aug 14, 2024

Ah, ok.

we should address issues with wrapping before adding more content

What is the solution for this? I mean, how should we deal with that?

@agjohnson
Copy link
Contributor

I don't have immediate suggestions for how to make the UX nice. There are CSS fixes that will avoid wrapping the icon and text like I showed though. But this will either require that the flyout content grows horizontally, clips horizontally, or wraps cleanly onto a second line. All of these options have their own drawbacks and affect UX in their own ways.

@humitos
Copy link
Member Author

humitos commented Aug 14, 2024

But this will either require that the flyout content grows horizontally, clips horizontally, or wraps cleanly onto a second line.

Can you give some directions on what are the CSS rules or documentation I have to read to implement any of these options? I can give it a try.

@agjohnson
Copy link
Contributor

Generally speaking, you'll need to look into flexbox rules to tune how these blocks flow and there are many rules that will affect how text inside these columns is displayed, wrapped, and truncated -- overflow/text-overflow, max-width, and a number of whitespace/word breaking rules.

Warning: flexbox is sufficiently complex that there is often no one correct answer.

@humitos
Copy link
Member Author

humitos commented Sep 10, 2024

@zanderle do you have experience with CSS rules/grid (e.g. flexbox) to work on this? I think that it could take me non-trivial time learning these concepts before being able to start working on this.

@zanderle
Copy link
Collaborator

@zanderle do you have experience with CSS rules/grid (e.g. flexbox) to work on this? I think that it could take me non-trivial time learning these concepts before being able to start working on this.

I haven't done a lot of work with grid, but I have a lot of experience with flexbox, so feel free to assign to me.

@agjohnson
Copy link
Contributor

I'll also echo some conversations we've had here and note that the issue we're facing here is that we had expanded the needed size of the flyout when adding:

  • The Read the Docs wordmark, in addition to an icon/logo
  • An icon before the version string

Both of these require more horizontal space, and a version selector and icon will make this menu longer still. I feel if we want the language in the collapsed flyout menu, we should do something different and probably should not use wordmark in the menu.

I don't feel the answer here is to expand the size of the flyout menu more, as it already has wrapping issues. I do think we should address issues with flexbox use and wrapping the flyout menu though.

@agjohnson
Copy link
Contributor

Also, it's worth noting here that we also dropped the dropdown caret on the right side of the menu. Without this, there isn't a visual cue that the flyout menu expands. This is more horizontal space we need to allocate.

To clarify, I'm feeling like we're making more work for ourselves by investing work into a 1.5 version of the flyout without a strong plan for design direction. I'd rather we do what we know works for now, skip more feature additions into this version of the flyout, and start working on a plan for a new flyout.

We talked about reducing the space in the flyout by removing the "Read the Docs"
word mark from the collapsed version of the flyout.

This will give us some room for icon+language implemented in
#360 and also to display a
dropdown (or similar) icon to communicate there are features behind it.
@humitos
Copy link
Member Author

humitos commented Oct 3, 2024

I merged the changes made in #392 and update this PR. I'm happy with the result of this iteration. Here is how it looks now:

Peek 2024-10-03 12-12

it's worth noting here that we also dropped the dropdown caret on the right side of the menu. Without this, there isn't a visual cue that the flyout menu expands. This is more horizontal space we need to allocate.

I opened #393 to track this work.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks great!

width: 106px;
margin-right: 20px;
max-width: 106px;
max-height: 22px;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want an explicit height vs. a max-height, so it's always a known height?

Copy link
Member Author

Choose a reason for hiding this comment

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

This comes from #392 and it's because these are dynamic values depending on the image used: logo or logo+wordmark. We can't use a fixed value for both images.

humitos added a commit that referenced this pull request Oct 10, 2024
We talked about reducing the space in the flyout by removing the "Read
the Docs" word mark from the collapsed version of the flyout.

This will give us some room for icon+language implemented in
#360 and also to display a
dropdown (or similar) icon to communicate there are features behind it.

![Peek 2024-10-03
12-04](https://github.com/user-attachments/assets/631ad184-1d57-40d5-b742-f845f853a503)
@humitos
Copy link
Member Author

humitos commented Oct 10, 2024

I've covered most of the feedback I received here and in a talk I had with @agjohnson. I think this is ready to move forward. I'm opening more issues to deal with small improvements.

it's worth noting here that we also dropped the dropdown caret on the right side of the menu. Without this, there isn't a visual cue that the flyout menu expands

We have this issue opened #393 and I also opened a PR with a POC.

With all three of these though, we'll need to have a better plan on how to handle long version names on the collapsed view.

I've opened #400 to deal with this.

@humitos humitos merged commit c958fa8 into main Oct 10, 2024
4 checks passed
@humitos humitos deleted the humitos/flyout-header-language branch October 10, 2024 10:56
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.

Flyout: display language when collapsed
6 participants