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

Only expand Checks list on PRs, not in popups #4130

Merged
merged 2 commits into from Mar 19, 2021
Merged

Only expand Checks list on PRs, not in popups #4130

merged 2 commits into from Mar 19, 2021

Conversation

jameschensmith
Copy link
Contributor

@jameschensmith jameschensmith commented Mar 19, 2021

Summary

Expands only the PR merge status list (and not the popups/dropdowns) to fit all items. This can be thought of as a fix for the popups/dropdowns.

Test URL(s)

Screenshot(s)

Long merge status list fixed

Long merge status list fixed

References

Fixes #4124.

@jameschensmith jameschensmith marked this pull request as ready for review March 19, 2021 05:06
@fregante
Copy link
Member

On second thought:

  • on the PR page, the section is part of the normal flow of the page, so I feel it's perfectly fine to leave it as infinite. It's already togglable, so it's doesn't make sense to limit it in anyway.
  • the popup’s awkwardly extends past the natural height of the page, so it makes sense to shorten that.

Maybe we can avoid changing the popup’s height altogether, and only leave max-height: none on the PR

Comment on lines 78 to 81
/* Expand merge status lists to fit more items */
.branch-action-item-simple > .merge-status-list, /* Dropdowns */
.branch-action-item.open > .merge-status-list { /* Pull requests */
max-height: 512px !important;
Copy link
Member

@fregante fregante Mar 19, 2021

Choose a reason for hiding this comment

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

Suggested change
/* Expand merge status lists to fit more items */
.branch-action-item-simple > .merge-status-list, /* Dropdowns */
.branch-action-item.open > .merge-status-list { /* Pull requests */
max-height: 512px !important;
/* Expand PR merge status list to fit more items */
:root .branch-action-item.open > .merge-status-list {
max-height: none;

We use :root to avoid !important, which is harder to override by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, okay. I saw the other !important uses and compared it to this single use of :root, and thought that was the desired route 😅 I'll address this 👍

@jameschensmith
Copy link
Contributor Author

On second thought:

  • on the PR page, the section is part of the normal flow of the page, so I feel it's perfectly fine to leave it as infinite. It's already togglable, so it's doesn't make sense to limit it in anyway.
  • the popup’s awkwardly extends past the natural height of the page, so it makes sense to shorten that.

Maybe we can avoid changing the popup’s height altogether, and only leave max-height: none on the PR

Okay, so it sounds like we should just keep what we have for the PR page, and filter out the dropdown/popup. I understand your request 😊 👍 I'll do my best to figure that out and get the change in!

@jameschensmith jameschensmith changed the title Limit merge status items to modest size Expand only PR merge status list to fit all items Mar 19, 2021
@jameschensmith
Copy link
Contributor Author

Updated the PR, title, and description to reflect the new request. This now covers addressing the popups/dropdowns, while keeping the PR merge status list infinite.

@fregante fregante changed the title Expand only PR merge status list to fit all items Only expand Checks list on PRs, not in popups Mar 19, 2021
@fregante fregante merged commit dbd18fa into refined-github:main Mar 19, 2021
@fregante
Copy link
Member

fregante commented Mar 19, 2021

Thank you for the PR and for following its template, James! 😁 🥨

@jameschensmith jameschensmith deleted the jameschensmith/fix/limit-merge-status-lists branch March 19, 2021 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Expand only PR merge status list to fit all items
2 participants