Fix dropdowns at bottom of patch page #2
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is mostly useful to avoid people from adding trailing whitespace to files.
Previously we'd include the ID of the commitfest in the URL of the patch. In 9f12a5e we introduced a stable URL for patches that would redirect to the one for the latest commitfest. This starts to use that URL as the valid only URL for a patch (with the previous URL redirecting to this one). The reasoning behind this is that the old approach resulted in N different URLs for each patch, which all showed the exact same patch information. The only difference between all these URLs would be the breadcrumb at the top of the page. The only benefit of that approach is that if you're on an old commitfest, and click a link there, then the breadcrumb will bring you back to where you came from. Since people rarely have a reason to browse closed commitfests, the that benefit seems pretty small. Especially because people can just as well press their browser back button, in that case. The problems that these N links cause seem much more impactful to most users: 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 of the commitfests that a patch has been part of. Those links were already rather useless, since all they effectively did was change the breadcrumb. But with this new commit, they wouldn't even do that anymore, and simply redirect to the current page. So now they start pointing to the commitfest itself, which seems more useful behaviour anyway.
The ID of a CF entry is the only stable identifier (people can change the name). That's why tooling such as CFbot uses the ID of the patch for a lot of things (including showing it on the cfbot overview page). Having it visible on the page is quite useful for debugging purposes In eee60a5 the ID was added to the title of the entry, but that made the the title of the page itself less prominent. So this is an attempt to have the information available, but not too prominently visible.
If the commitfest entries are sorted by a column clicking the header again will now remove the sort. In a future commit, it would be nice to also support for reverse sorting.
It was pretty confusing that clicking the patch name header would sort by created time, grouped by topic. This makes that sort by name.
Sorting order impacts closed patches too. So not showing the arrow that indicates sort direction on that header is confusing. While we're at it, we might as well allow people to toggle sorting direction using that header too. This also automatically fixes the problem that the cell in the closed patches header did not contain any title at all.
polkerty
changed the title
|
Code looks good, I'll try this out soonish.
Opened #3 with some initial attempt at upgrading to Bootstrap 5. |
JelteF
reviewed
Jan 13, 2025
| @@ -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"> | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
ah, 🤕 thank you for catching this. Here is the revised PR: #4
JelteF
force-pushed
the
master
branch
7 times, most recently
from
7cd9132 to
ebbf020
Compare
polkerty
force-pushed
the
jbrazeal/dropdowns
branch
from
2c24c31 to
79a48c8
Compare
polkerty
force-pushed
the
jbrazeal/dropdowns
branch
from
83c03a1 to
0957e96
Compare
|
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before:
Suppose you click on one of these buttons at the bottom of the patch page:
Then the drop down opens below the fold of the page, and you have to scroll down to see it:
Now:
These dropdowns open up, so you don't have to scroll. (This only affects specific dropdowns that we tag.)
Note: a more extensive solution would be upgrading to Bootstrap 4, which, I hear, can dynamically choose where a dropdown opens.