- 🇳🇱Netherlands yoroy
Had a quick look at how Google docs and Gitlab wiki pages do this. Both explicitly *remove* the UI for editing when showing an older revision of a document.
Google docs adds a primary button to revert to this revision
Gitlab wiki only points back to the revision history overview
So I tend to agree with @29 in that we should not add the standard set of node local actions. Adding the option to directly restore to the currently shown revision seems useful. How's this for a new title for this issue?
- Status changed to Needs review
over 1 year ago 9:15pm 8 August 2023 - last update
over 1 year ago 29,953 pass - Status changed to Needs work
over 1 year ago 5:39pm 9 August 2023 - 🇳🇿New Zealand john pitcairn
Based on the code in this patch, I've implemented a revision pager of sorts as secondary tasks:
I don't really see a problem with keeping the node canonical tabs in place if revision-related tasks are always secondary. I'm considering adding a "Revert" option to this.
I originally had something like this way back in Drupal 6 and remember my editors liking it. The revision label changes to "Latest revision (xx)" or "Published revision (xx)" when on those revisions.
But maybe making navigating between revisions easier is something worth considering here? It's easy to get lost if you have to keep going back to the full revision list.
- last update
over 1 year ago 29,672 pass - Status changed to Needs review
about 1 year ago 12:55pm 10 January 2024 - 🇮🇳India djsagar
Verified and tested #38 patch applied successfully and resolved.
Steps to reproduce
1. Install Drupal Standard profile
2. Set Claro as the default theme.
3. Create basic page content item and save
4. Edit same page node, create new revision
5. Go to node's revisions tab, node/1/revisions
6. Click the initial node revision to view it, /node/1/revisions/1/view
7. Observe no tabs present on revision pathFor reference:
Before patch
After patch
Results:
After applying the patch, the Node Local Tasks tabs are visible on node revisions.
RTBC ++ - Status changed to Needs work
about 1 year ago 1:02pm 10 January 2024 - 🇺🇸United States smustgrave
Please please read the tags and issue summary, this was tagged for tests which are still needed.
- 🇮🇳India djsagar
@smustgrave My apologies. I will adhere to tags moving forward.
- 🇬🇧United Kingdom farse
I can confirm that the patch in https://www.drupal.org/project/drupal/issues/2885278#comment-15144519 📌 Add useful local tasks tabs when viewing a node revision Needs work works for Drupal 10.1.6 version.
- First commit to issue fork.
- 🇬🇧United Kingdom oily Greater London
I cannot apply patch #37 or #38 to the fork for Drupal 11.x. A reroll is required
- 🇬🇧United Kingdom oily Greater London
I have added commits to the issue fork. I have used an existing functional test function but added assertions. There is now a failing test that will hopefully pass once the the patch(es) are applied.
I tried re-rolling both patches at #37 and #38 without success. It would be good if someone who has worked on the fix could add commits to the issue fork to see if they can make the test pass.
- First commit to issue fork.
- 🇬🇧United Kingdom oily Greater London
Changing the status since the functional test assertions are passing in the pipeline following #50.
- 🇬🇧United Kingdom oily Greater London
I'm not sure why but the MR does not contain the new test assertions. They were added to the fork in an old commit. Will need to add them once again to prove that the 'fix' makes the test pass.
- 🇬🇧United Kingdom oily Greater London
Not sure if phpunit has been upgraded but my local phpunit has stopped working. Seems the test assertions for this issue are also not running in the pipeline. The code is added to the MR and the MR should be ready to review (if phpunit is working).
- 🇬🇧United Kingdom oily Greater London
I have created a new FunctionalJavascript test using the Claro (admin) theme. It is passing with the MR applied. Changing the status to 'Needs Review'.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇬🇧United Kingdom oily Greater London
Test is moved to the node module.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇬🇧United Kingdom oily Greater London
After running into rebasing problems, created a new 11.x branch and added the code changes to it.
Several tests need to be fixed.
- First commit to issue fork.
- 🇮🇳India ramprassad
I have committed the changes to fix the MR, now the failing tests are fixed. Please check
- 🇬🇧United Kingdom oily Greater London
The tests seem fixed now, pipeline runs all green except for the test-only test: fails like it should.
That seems to leave several out-of-scope typehints that need to be removed from the MR.
- 🇺🇸United States smustgrave
May need to think approach as not seeing why we are adding comment hooks into node. Should try and keep things separate if possible.