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

hlist: Make columns collapse when viewport is narrow #1109

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Blendify
Copy link
Member

@Blendify Blendify commented Apr 2, 2021

When the items of the hlist cannot fit into the viewport a collumn is split into a new row until the content fits.

This dramically improves mobile experience, for example https://docs.blender.org/api/current/bpy.types.AlphaUnderSequence.html will introduce horizonal scrolling on the whole viewport which causes visual glitchs with other content.

Fixes #1108

When the items of the hlist cannot fit into the viewport a collumn is split into a new row until the content fits.

This dramically improves mobile experience, for example https://docs.blender.org/api/current/bpy.types.AlphaUnderSequence.html will introduce horizonal scrolling on the whole viewport which causes visual glitchs with other content.

Fixes #1108
@Blendify Blendify added this to the 1.0 milestone Apr 2, 2021
@Blendify Blendify requested review from agjohnson, stsewd and a team April 2, 2021 02:30
@Blendify
Copy link
Member Author

Blendify commented Apr 2, 2021

Before:
image

After:
image

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This fix seems good on other browsers, but IE11 does have problems with the flex box layout. If we want to merge this before dropping IE11 support, flex box can be used to some degree in IE10+11. So far, IE10+11 support has been the reason we haven't used flex layout more.

display: flex
flex-flow: row wrap
td
margin-right: auto
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, this theme does not use flex box layout because we have not officially dropped support for IE11, or IE10 for that matter. There is limited support for flex box in IE10+11, but you will need to add more rules.

This PR does break hlists on IE11:

image

This is what IE11 looks like on latest release, for comparison:

image

@Blendify
Copy link
Member Author

Blendify commented Apr 2, 2021

I would be in favor of dropping ie 11, the bug you show is avoidable on the user side if they set figwidth on the figure directive. The bug you show is also not present on all ie11 using browser stack, on windows 7 and ie 11 it displayed correctly for me. I bet the majority of users that use ie 11 are on windows 7.

There are also already issues with ie11 with the theme (see the logo) so maybe we go ahead and formally drop compatibility now. Also, the bug you show is similar to #1050 which affected all browsers maybe it is acceptable to introduce a minor regression. If we don't drop ie11 for 1.0 it might be a while until we release 2.0 (dropping ie should be part of a major release).

Does adding display: -ms-flexbox fix the bug? According to the docs I have read flex-flow is supported on ie11 without a prefix. If the solution is that simple, I can just add that and we can hold off on dropping ie11 a little longer.

@Blendify Blendify requested a review from agjohnson April 5, 2021 17:20
@agjohnson
Copy link
Collaborator

agjohnson commented Apr 5, 2021

Does adding display: -ms-flexbox fix the bug?

I did not test this. In theory it might though, IE11 requires a prefix on flex box rules for what support it did have.

There are also already issues with ie11 with the theme (see the logo)

This is a minor display issue for now, it probably doesn't warrant dropping ie11 support immediately by itself.

The bug you show is also not present on all ie11 using browser stack, on windows 7 and ie 11 it displayed correctly for me.

I tested on saucelabs ie11 + win7 (edit: win10), against the PR build output.

@agjohnson
Copy link
Collaborator

agjohnson commented Apr 5, 2021

Most recent update seems to have introduced vertical spacing between elements and did not fix the issue with overflow/wrap on the hlist caption:

image

@agjohnson
Copy link
Collaborator

agjohnson commented Apr 5, 2021

Oops, I take that back. It also looks like you didn't rebuild assets yet, only the SASS source was updated. Trying this change manually does not alter the display however. Also, tested on win7 and win10 and no difference on ie11

@Blendify
Copy link
Member Author

Blendify commented Apr 5, 2021

Most recent update seems to have introduced vertical spacing between elements

I think that is a docutils 0.17 issue, rebased the patch on master and updated assets so hopefully, work as expected.

@Blendify Blendify modified the milestones: 1.0, 1.1 Apr 8, 2021
Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

According to our own docs, we only partly support IE11 and will remove support in 2.0.

See: https://sphinx-rtd-theme.readthedocs.io/en/stable/development.html#supported-browsers

From this, I would gather that this change is entirely fine for 1.1, since it will not introduce a "major bug".

@agjohnson
Copy link
Collaborator

agjohnson commented Aug 29, 2022

It would probably make more sense to revisit in 2.0, when we do drop IE11 support. Feel free to add IE11 support if you'd rather aim for a sooner release.

@agjohnson agjohnson modified the milestones: 1.1, 2.0 Aug 29, 2022
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.

Use flexible layout for hlist directives
3 participants