Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Make status bar accessible at 400% zoom by hiding items with priority of zero (default)#14854

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@j264415
Copy link
Contributor

@j264415j264415 commentedJul 19, 2023•
edited by krassowski
Loading

References

Code changes

  • newpriority argument is added toIStatusBar.IRankItem, which defaults to0
  • when window is narrow (less or equal to 630 pixels, which is a heuristic for 400% zoom) the items with priority of zero get hidden so that the higher priority items remain visible

User-facing changes

Currently with 400% zoom at 1280px the status bar elements overlap each other which hinders usability. Unrequired elements have been removed and the remaining elements no longer overlap at 320px apparent width (1280px/400%).

This means that when a window is below 320px apparent width, the kernel title, document name and the processor icons will be hidden from view to allow space for the simple switch and the line-column number to be fully visible and accessible. If future text elements are added these will also be hidden unless they are given the priority-zoom class.

Backwards-incompatible changes

Extensions which add custom status bar items may want to set custom priority greater than one if these need to be visible at narrow screens.

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch onbinder, follow this link:Binder

@github-actionsgithub-actionsbot added pkg:codeeditor pkg:statusbar tag:CSSFor general CSS related issues and pecadilloes Design System CSS labelsJul 19, 2023
@j264415j264415 marked this pull request as ready for reviewJuly 19, 2023 15:33
@t03857785t03857785 mentioned this pull requestJul 21, 2023
@j264415
Copy link
ContributorAuthor

j264415 commentedAug 2, 2023•
edited
Loading

Please could this PR be reviewed.@krassowski

@j264415
Copy link
ContributorAuthor

Please could this PR be reviewed.@gabalafou

Copy link
Contributor

@gabalafougabalafou left a comment•
edited
Loading

Choose a reason for hiding this comment

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

While I think this PR has some great ideas, it also makes me uncomfortable because I feel like it's trying to achieve at the CSS level what should be achieved at the app/JS level.

For example, the StatusBar class provides aregisterStatusItem method. This method is used by extensions, including built-in extensions, to add items to the status bar, like so:

// Add the status item to the status bar.statusBar.registerStatusItem(lineColItem.id,{  item,align:'right',rank:2,isActive:()=>!!item.model.editor});

It seems to me that this is the point at which something like your "priority-zoom" property should be passed.

I also question some of the decisions about what is and isn't priority in this PR. To me the notification icon is a better use of space than the simple-mode toggle—both because the toggle takes up a lot of space and also because the same command can be accessed in two other ways, via the menu and via the command palette; also there are other more obvious cues about when the app is in simple mode or the not, so I'm not sure how much value is added by always showing that state in the status bar.

I also question how effective this approach is because, for example, here's a screenshot on my laptop with the UI zoomed to 400% and it shows that the changes in this PR have done nothing to help because my laptop screen at 400% does not trigger the at-media rule:



I agree that the status bar is too cluttered at narrow width and/or high zoom. But I'm not sure this is the way to solve it. Or maybe this is the right direction, maybe the items in the status bar need to indicate some priority-level, and then if the status bar doesn't have enough width for all of the items, it starts to remove the lower-priority ones. But I think that functionality probably needs to implemented at the level of the StatusBar TypeScript class.

@j264415
Copy link
ContributorAuthor

j264415 commentedAug 31, 2023•
edited
Loading

Hi@gabalafou looking at the comment you made above about implementing the changes that you want for the 400% zoom through the app/JS level and not CSS; I have made the relevant code changes required to achieve this.
Please could you review these changes.
Thank you

@j264415
Copy link
ContributorAuthor

please update galata snapshot

@j264415
Copy link
ContributorAuthor

please update documentation snapshots

@github-actions
Copy link
Contributor

Documentation snapshots updated.

@j264415
Copy link
ContributorAuthor

Hi@gabalafou could you please review the changes I have made from your feedback.
Thank you.

private_isWindowZoomed=()=>{
// Check is the window width is less than or equal to 630px as this is when the staus bar become unaccessible to the user.
// Works across all screen resolution.
returnwindow.innerWidth<=630;
Copy link
Member

@krassowskikrassowskiDec 14, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Question from@gabalafou on the call: is there a justification for this specific number?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This number is because this is when the status bar starts to become inaccessible. The comment above has been reviewed by@gabalafou and changed to make the reasoning of this number clearer.

@krassowskikrassowski added this to the4.1.0 milestoneDec 14, 2023
Copy link
Contributor

@gabalafougabalafou left a comment

Choose a reason for hiding this comment

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

Thank you for working on this! I added just a few suggestions to reflect the last changes in the comments and method names

j264415 reacted with thumbs up emoji
j264415and others added4 commitsDecember 15, 2023 09:31
Co-authored-by: gabalafou <gabriel@fouasnon.com>
Co-authored-by: gabalafou <gabriel@fouasnon.com>
Co-authored-by: gabalafou <gabriel@fouasnon.com>
@j264415
Copy link
ContributorAuthor

Thank you for working on this! I added just a few suggestions to reflect the last changes in the comments and method names

Thank you for reviewing this !

@gabalafou
Copy link
Contributor

gabalafou commentedDec 15, 2023•
edited
Loading

This looks good to go from my perspective. The only check failing is UI Tests / Visual Regression, which may be unrelated to this PR.

If there are no objections, I will merge it in on Monday, December 18.

j264415 reacted with thumbs up emoji

@krassowski
Copy link
Member

This looks good to go from my perspective.

Can you approve it? It seems there is still a an "X" from you. I often sort by approved when merging things before release.

image

The only check failing is UI Tests / Visual Regression, which may be unrelated to this PR.

I think they are unrelated, but I kicked it

If there are no objections, I will merge it in on Monday, December 18.

I might merge it earlier to fit it inbeta.0.

Copy link
Member

@krassowskikrassowski left a comment

Choose a reason for hiding this comment

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

Thank you@j264415!

j264415 reacted with thumbs up emoji
@krassowskikrassowski changed the titleMade Status bar accessible at 400% zoomMake status bar accessible at 400% zoom by hiding items with priority of zero (default)Dec 20, 2023
@krassowskikrassowski merged commit9956d5c intojupyterlab:mainDec 20, 2023
@krassowski
Copy link
Member

@j264415 I updated the top-level comment's Code Changes and Backwards-incompatible changes sections. Please take note for future PRs that we need to keep them aligned with the content of the PR and they can be brief, as long as the key take-away messages are conveyed :)

@j264415 can you please follow with another pull request adding a note to extension migration guide about the newpriority attribute and what it entails for extension developers (see my note in Backwards-incompatible changes section, feel welcome to expand on this with an example).

j264415 reacted with thumbs up emoji

@j264415
Copy link
ContributorAuthor

@j264415 can you please follow with another pull request adding a note to extension migration guide about the newpriority attribute and what it entails for extension developers (see my note in Backwards-incompatible changes section, feel welcome to expand on this with an example).

@krassowski All done. This is the PR:#15556

@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsDec 25, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@krassowskikrassowskikrassowski approved these changes

@fcollonvalfcollonvalAwaiting requested review from fcollonval

+1 more reviewer

@gabalafougabalafougabalafou requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@j264415j264415

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

Status bar at 400% zoom

4 participants

@j264415@gabalafou@krassowski@fcollonval
[8]ページ先頭
©2009-2025 Movatter.jp