Quick Tags, Trim Reblogs: Fix features on accounts with new footer variants #1802
Conversation
|
oh neat! drafts have yet another different footer implementation. sure. |
There was a problem hiding this comment.
This is a prime example of a pull request that can be split up for easier reviewing. While the fixes both rely on the new utils, they don't rely on each other, which makes reviewing this a minimum of three units of review work:
- Make sure the util code is high-quality. Something that is meant to be reused needs to be very stable so that maintenance on it doesn't break everything that relies on it.
- Make sure Quick Tags works with the new util.
- Make sure Trim Reblogs works with the new util.
With these three things being in the same pull request, that puts a burden on the reviewer to keep track of which things are verified and which are not. This is surprisingly hard to do when it's not your own code! Instead, this could be one pull request per unit of reviewer work:
- Open PR targeting the main branch that only adds the new util
- Draft PR targeting the util branch that fixes Quick Tags
- Draft PR targeting the util branch that fixes Trim Reblogs
When the util PR is approved, you'll still be able to test that the fix PRs still work before merging it, since they will be based on the util branch and inherit any changes introduced over the course of the util PR's review cycle. If something is broken and the util code needs to be revisited, that's fine, we can dismiss the util approval and continue collaborating on that. If something is broken and the code for one or both fixes needs to be revisited, on the other hand, the util code can still be merged first in that scenario, because we've already established that we have confidence in the util code.
I suppose "confidence" is the easiest way of thinking about what the review process is really for, and why it's better to make changes in as small steps as possible. The more pieces to a PR there are, while it's the same effort to validate each piece individually, it's much harder to be confident that I haven't missed something in how they all fit together.
|
Interesting—I imagine that review process being more difficult, but of course that's entirely guesswork and you have the experience to know the actual answer! In the workflow you've suggested, I'm making feature branches in this repository rather than in my fork. Would you like me to start doing that? (In cases where stacked PRs would make sense, presumably, though I could also start doing that for more than just that. Yes I would make sure to delete feature branches after merge in that case, don't worry :P) |
Hmmm, I forgot that you weren't already doing that, to be honest. If you'd prefer to keep pushing to your own fork first, the workflow could be modified wherein the fix PRs would still be based on the util PR's commits, but would need to be rebased manually every time the util PR changes (or gets merged). Up to you! If you do start pushing branches here, I would like the branches to be prefixed for tidiness purposes... the "norm" I'm used to is prefixing with one's GitHub username for purposes of clear ownership (e.g. |
I think—don't quote me on this—that the fix PRs would still need to be rebased manually if they were on this repo and stacked and we merged the util PRs first? Okay, I'm not actually super confident about that, but iirc when I've done that, the fact that the main branch has a squashed commit equivalent to multiple commits on the feature branch for the fix PR makes a merge conflict that doesn't get auto resolved. Will have to play with that on another repo now, because I'm curious. Anyway, not a big deal. (I've seen instructions for stacked PRs in which, once everything is approved, you merge the fix PRs into the util PR, but I'd rather have them separate in the history if they're separate PRs with separate threads to link to.)
Yeah, I like that. I'll probably continue to keep many things on my fork because I use a ton of branches, but I think there could be places where it could be appropriate. |
I forgot that git is stupid like that 🫶 so I suppose there wouldn't be much benefit to pushing your branches to this repo?
Yeah, I've seen that in action and I'm not a fan of it. I've been assigned to review a PR that targets another PR branch, ascertained the before-and-after behaviours, and approved the PR for being a good change... only for the target branch to be updated as part of its PR's review cycle, and for the PR which I approved to need re-validating as a result. |
It would allow any contributor (so... you) to make a PR targeting the feature branch, which could be appropriate in certain circumstances, I imagine. If you had a larger change proposal or multiple change proposal options to discuss, say (sure, one can write diffs inline in markdown, but it's pretty inefficient compared to just pushing a branch and saying "how about this.") Niche, but not useless. I do think it does increase clarity when a PR targets a feature branch vs just writing "this PR is based on/contains the commits of xyz PR" in the description! And hey, maybe doing a merge commit or rebase actually does work in these cases. |
|
Oh! Wait, no: one critical difference with PRs that target feature branches is that the github "files changed" view (which I personally use quite a bit for PR review) will show only the changes in the fix PR, rather than including all of the util PR changes. So, anyway: would you like this PR to be split in the way we've been discussing, or to leave it as-is in this case because you've already begun reviewing it? |
|
Let's proceed with the PR as-is; I'd like to release a version with all the footer fixes included before EOD. |
There was a problem hiding this comment.
- Control variant
- Confirm that Quick Tags/Trim Reblogs buttons are added to regular editable posts.
- Confirm that Quick Tags/Trim Reblogs buttons are added to drafts.
- Confirm that Quick Tags/Trim Reblogs buttons are added to queued posts.
- Confirm that Quick Tags/Trim Reblogs buttons have reasonable spacing and tap targets in the desktop/tablet layout.
- Confirm that Quick Tags/Trim Reblogs buttons have reasonable spacing and tap targets in the mobile layout.
- Confirm that Quick Tags/Trim Reblogs buttons are not added to community posts.
- Confirm that Quick Tags/Trim Reblogs buttons are not added to asks, private answers, and submissions in the inbox.
- Combined notes test variant
- Confirm that Quick Tags/Trim Reblogs buttons are added to regular editable posts.
- Confirm that Quick Tags/Trim Reblogs buttons are added to drafts.
- Confirm that Quick Tags/Trim Reblogs buttons are added to queued posts.
- Confirm that Quick Tags/Trim Reblogs buttons have reasonable spacing and tap targets in the desktop/tablet layout.
- Confirm that Quick Tags/Trim Reblogs buttons have reasonable spacing and tap targets in the mobile layout.
- Confirm that Quick Tags/Trim Reblogs buttons are not added to community posts.
- Confirm that Quick Tags/Trim Reblogs buttons are not added to asks, private answers, and submissions in the inbox.
- Split notes test variant
- Confirm that Quick Tags/Trim Reblogs buttons are added to regular editable posts.
- Confirm that Quick Tags/Trim Reblogs buttons are added to drafts.
- Confirm that Quick Tags/Trim Reblogs buttons are added to queued posts.
- Confirm that Quick Tags/Trim Reblogs buttons have reasonable spacing and tap targets in the desktop/tablet layout.
- Confirm that Quick Tags/Trim Reblogs buttons have reasonable spacing and tap targets in the mobile layout.
- Confirm that Quick Tags/Trim Reblogs buttons are not added to community posts.
- Confirm that Quick Tags/Trim Reblogs buttons are not added to asks, private answers, and submissions in the inbox.
|
Worth noting: it doesn't look like queued posts or the inbox have any new post footer treatment yet, but we can expect those to be coming down the pipe sometime. Hopefully the Edit button detection logic that's working on the new treatment for Drafts will also work in the Queue and on unpublished submissions... I don't remember what buttons we can expect for unpublished asks. |
Description
This is a way to fix Quick Tags and Trim Reblogs, the features that add control buttons to posts, on accounts with the two new post footer variants. Control buttons are added in an additional secondary row above the footer control buttons, as in the current footer layout, which must be added by a new util function.
If we use this method, we should make sure that this code is easy to copy into and can be compatible with XKit 7 in case we ever get around to performing the same fix on it.
Note that this CSS might be a little imprecise (for example, I split the difference between some mobile/non-mobile spacing values, iirc; I was in a bit of rush to get an MVP out). We're not directly reproducing a layout that exists; feel free to weigh in.
Testing steps
On all footer variations:
Note: I haven't done a full round of testing on this after my last few changes, unless I've crossed out this line in the description.