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

Fix dropdowns at bottom of patch page#2

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

Closed

Conversation

polkerty
Copy link
Contributor

@polkertypolkerty commentedJan 13, 2025
edited
Loading

Before:
Suppose you click on one of these buttons at the bottom of the patch page:
image

Then the drop down opens below the fold of the page, and you have to scroll down to see it:
image

image

Now:
These dropdowns open up, so you don't have to scroll. (This only affects specific dropdowns that we tag.)

image

Note: a more extensive solution would be upgrading to Bootstrap 4, which, I hear, can dynamically choose where a dropdown opens.

This is mostly useful to avoid people from adding trailing whitespace tofiles.
Previously we'd include the ID of the commitfest in the URL of thepatch. In9f12a5e we introduced a stable URL for patches that wouldredirect to the one for the latest commitfest. This starts to use thatURL as the valid only URL for a patch (with the previous URL redirectingto this one).The reasoning behind this is that the old approach resulted in Ndifferent URLs for each patch, which all showed the exact same patchinformation. The only difference between all these URLs would be thebreadcrumb at the top of the page.The only benefit of that approach is that if you're on an oldcommitfest, and click a link there, then the breadcrumb will bring youback to where you came from. Since people rarely have a reason to browseclosed commitfests, the that benefit seems pretty small. Especiallybecause people can just as well press their browser back button, in thatcase.The problems that these N links cause seem much more impactful to mostusers:1. If you click an old link to a cf entry (e.g. one in the email   archives), then the breadcrumb will contain some arbitrarily old   commitfest. It seems much more useful to have the breadcrumb show the   commitfest that the patch is currently active in (or got   committed/rejected in).2. Places that use the stable URLs require an extra round-trip to   actually get to the patch page.3. It's a bit confusing that old pages of a patch still get updated with   all the new information, i.e. why have all these pages if they   contain the exact same content.4. Problem 3 is generally also bad for Search Engine Optimization (SEO),   for now we don't care much about that though.Finally this also changes the links on the patch page itself for each ofthe commitfests that a patch has been part of. Those links were alreadyrather useless, since all they effectively did was change thebreadcrumb. But with this new commit, they wouldn't even do that anymore,and simply redirect to the current page. So now they start pointing tothe commitfest itself, which seems more useful behaviour anyway.
The ID of a CF entry is the only stable identifier (people can changethe name). That's why tooling such as CFbot uses the ID of the patch fora lot of things (including showing it on the cfbot overview page).Having it visible on the page is quite useful for debugging purposesIneee60a5 the ID was added to the title of the entry, but that made thethe title of the page itself less prominent. So this is an attempt tohave the information available, but not too prominently visible.
If the commitfest entries are sorted by a column clicking the headeragain will now remove the sort. In a future commit, it would be nice toalso support for reverse sorting.
It was pretty confusing that clicking the patch name header would sortby created time, grouped by topic. This makes that sort by name.
Sorting order impacts closed patches too. So not showing the arrow thatindicates sort direction on that header is confusing. While we're at it,we might as well allow people to toggle sorting direction using thatheader too.This also automatically fixes the problem that the cell in the closedpatches header did not contain any title at all.
@polkertypolkerty changed the titleFix dropdowns at bottom of pageFix dropdowns at bottom of patch pageJan 13, 2025
@JelteF
Copy link
Collaborator

JelteF commentedJan 13, 2025
edited
Loading

Code looks good, I'll try this out soonish.

Note: a more extensive solution would be upgrading to Bootstrap 4, which, I hear, can dynamically choose where a dropdown opens.

Opened#3 with some initial attempt at upgrading to Bootstrap 5.

@@ -3,15 +3,15 @@

<div class="btn-group">
<a class="btn btn-default dropdown-toggle" data-toggle="dropdown" href="#">Comment/Review <span class="caret"></span></a>
<ul class="dropdown-menu">
<ul class="dropdown-menu dropdown-menu__up">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding this extra class to the patch_commands.inc is problematic. Because this file is included both at the top and the bottom of a patch. At the top of the patch you want the dropdown to go down (like what is happening in prod now), but at the bottom of the patch you want it to go up (like what you implemented in this PR).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

ah, 🤕 thank you for catching this. Here is the revised PR:#4

@JelteFJelteFforce-pushed themaster branch 7 times, most recently from7cd9132 toebbf020CompareJanuary 14, 2025 23:21
@polkerty
Copy link
ContributorAuthor

I'm going to close this due to some git history difficulties and re-open. However, I have an update that'll fix this - thank you for catching.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@JelteFJelteFJelteF left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@polkerty@JelteF

[8]ページ先頭

©2009-2025 Movatter.jp