Suggested commit message:

Issue #1340242 by ancapaaron, gaurav.pahuja, mortendk, Pradnya Pingat, gnuget: add an arrow for the book up

Problem/Motivation

This came up during the markup & css discussion #736604: book module now without clearfix & better markup + accessibility love
how come that there's back & forward arrows but not an up arrow ?

Proposed resolution

Add the up arrow.

Remaining tasks

None

User interface changes

Added the symbol "^" in the up arrow in the book navigation links.

API changes

None

Data model changes

None

This is how this looks right now

Desktop

Mobile

This is how this looks with the patch

Firefox Desktop

Chrome Desktop

Safari Desktop

Chrome mobile

Firefox mobile

CommentFileSizeAuthor
#59 olivero-before-after.png24.04 KBkomalk
#59 umami-before.png30.25 KBkomalk
#59 umami-After.png35.19 KBkomalk
#59 seven-before.png20.57 KBkomalk
#59 seven-After.png19.76 KBkomalk
#59 claro-Before.png20.92 KBkomalk
#59 claro-After.png24 KBkomalk
#59 Bartik-After.png21.55 KBkomalk
#59 Bartik-Before.png19.54 KBkomalk
#53 1340242-53.patch5.25 KBkomalk
#50 before-patch.png22.63 KBkomalk
#50 after-patch.png23.33 KBkomalk
#50 interdiff_46-50.txt4.58 KBkomalk
#50 1340242-50.patch5.21 KBkomalk
#46 1340242-46.patch848 bytessarvjeetsingh
#45 Screen Shot 2020-09-30 at 8.54.16 AM.png27.66 KBmindbet
#45 Screen Shot 2020-09-30 at 8.55.13 AM.png27.43 KBmindbet
#28 add_an_arrow_for_the-1340242-28.patch3.49 KBgnuget
#28 interdiff-25-28.txt2.67 KBgnuget
#28 firefox-after-mobile.png8.49 KBgnuget
#28 chrome-after-mobile.png4.1 KBgnuget
#28 safari-after-desktop.png9.44 KBgnuget
#28 chrome-after-desktop.png10.18 KBgnuget
#28 firefox-after-desktop.png9.84 KBgnuget
#28 before-mobile.png8.12 KBgnuget
#28 desktop-before.png8.81 KBgnuget
#25 interdiff.txt617 bytesgaurav.pahuja
#25 1340242-25.patch2.75 KBgaurav.pahuja
#23 interdiff.txt449 bytesgaurav.pahuja
#23 1340242-23.patch2.75 KBgaurav.pahuja
#22 interdiff-1340242-12-19.txt1.38 KBgnuget
#19 1340242-comment18.patch2.66 KBancapaaron
#18 bartik-mobile-before-patch.png8.21 KBgnuget
#12 1340242-comment9.patch2.04 KBancapaaron
#10 coding_standard_issue_resolved-1340242-#10.patch1.27 KBPradnya Pingat
#9 align.png29.04 KBgnuget
#8 1340242-comment7.patch1.67 KBancapaaron
#6 1340242.patch1.26 KBancapaaron
up navigation.jpg95.21 KBmortendk
up navigation bartik.jpg50.65 KBmortendk
book-uparrow.patch9.39 KBmortendk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

Issue summary: View changes

Good question... Up arrows make sense to me. But don't know why this patch is so big. It's mostly just inserting <b>^</b> right?

kattekrab’s picture

Status: Active » Needs work

This needs a reroll.

Also, it's a good idea.

@mortendk it's still assigned to you, did you ever see @mgifford's question?

I'm pushing to needs work for the reroll - but I think this was languishing as active when it shoulda been needs review. Needs more eyeballs. :)

mgifford’s picture

Assigned: mortendk » Unassigned

Unassigning stale issue. Hopefully someone else will pursue this.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

gnuget’s picture

Issue tags: +Novice
ancapaaron’s picture

I am giving this is a shot from drupalcon New Orleans as a first time sprinter. I have created a patch using git that adds in the up arrow and a break to display the up aarow as shown in the picture.

gnuget’s picture

Hi ancapaaron

Thanks for your work on this issue.

+++ b/core/modules/book/templates/book-navigation.html.twig
@@ -43,7 +43,7 @@
+          <a href="{{ parent_url }}" title="{{ 'Go to parent page'|t }}">^<br>{{ 'Up'|t }}</a>

Can we avoid the use of <br> and use styles instead? also, taking ideas from #0 , can we wrap the ^ with <b></b>?

ancapaaron’s picture

Hi gnuget thank you for the feedback. I have created another patch based on the feedback, and have wrapped the arrow in <b></b>, and went ahead and changed the b encapsulated in the center navigation to display: block so that it appears above the "up" text.

gnuget’s picture

FileSize
29.04 KB

This is looking good! Thanks.

some comments:

+++ b/core/themes/classy/css/components/book-navigation.css
@@ -38,3 +38,7 @@
+.book-pager__item--center b {
+	margin-bottom: -.6em;
+	display: block;
+}

According to our coding standards we must use spaces instead of tabs to indent the code, here you are using tabs.

Can you please change it?

And

Now the next and prev links aren't aligned anymore. (see the screenshot)
align

Also, the next time to you update the patch, please change the status to "needs review" to let the bot run the tests against the patch.

Thanks!

Pradnya Pingat’s picture

Assigned: Unassigned » Pradnya Pingat
Status: Needs work » Needs review
FileSize
1.27 KB

Resolved coding standard issue.

Status: Needs review » Needs work

The last submitted patch, 10: coding_standard_issue_resolved-1340242-#10.patch, failed testing.

ancapaaron’s picture

Hi this patch should address all the problems with comment 9. Coding standards should be followed, and alignment issues should be taken care of via CSS. Thanks again for the help gnuget.

ancapaaron’s picture

Status: Needs work » Needs review

The last submitted patch, book-uparrow.patch, failed testing.

The last submitted patch, 6: 1340242.patch, failed testing.

The last submitted patch, 8: 1340242-comment7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 1340242-comment9.patch, failed testing.

gnuget’s picture

Assigned: Pradnya Pingat » Unassigned
FileSize
8.21 KB

Hi @ancapaaron

This is almost ready, some nitpicks:

+++ b/core/themes/classy/css/components/book-navigation.css
@@ -38,3 +37,7 @@
+.book-pager__item--center b {
+  margin-bottom: -.6em;
+  display: block;
+}
\ No newline at end of file
diff --git a/core/themes/classy/templates/navigation/book-navigation.html.twig b/core/themes/classy/templates/navigation/book-navigation.html.twig

We leave an empty line in the end of the file, that is why git is complaining. Can you please add it?

This change is going to need to update our tests please check the file: core/modules/book/src/Tests/BookTest.php line 253, you will found this:

    if ($up) {
      /** @var \Drupal\Core\Url $url */
      $url = $up->urlInfo();
      $url->setOptions(array('attributes' => array('title' => t('Go to parent page'))));
      $this->assertRaw(\Drupal::l('Up', $url), 'Up page link found.');
    }

Basically, we need to update the \Drupal::l('Up', $url) text and add the <b>^</b> there in order to make this match again with what the test expects.

And finally, I did a manual revision and we have problems in mobile with this patch, please see the screenshots below.

This is how this looks right now:

This is how this looks with your patch:
Only local images are allowed.

Great job so far, thanks!

ancapaaron’s picture

This latest patch should fix the display issue on mobile, take care of the test update, and add the newline back to the end of the css file so that git does not complain. Thank you again gnuget!

ancapaaron’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: 1340242-comment18.patch, failed testing.

gnuget’s picture

FileSize
1.38 KB

Ok, some comments here.

.book-pager__item--next {
  text-align: right; /* LTR */
  width: 43%;
}

We need to keep the same size at: .book-pager__item--next and .book-pager__item--previous so if you are going to edit the width of one of these you need to do the same change in the other.

Right now one is 43% and the other one is 45%, can you please make them have the same size?

Also, you need provide an interdiff every new iteration of the patch that makes it easier for us to review it. I will do it for you this time, but please provide one the next time.

You can see how to do an interdiff here: https://www.drupal.org/documentation/git/interdiff

Thanks!

gaurav.pahuja’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
2.75 KB
449 bytes
gnuget’s picture

Status: Needs review » Needs work
+++ b/core/themes/classy/css/components/book-navigation.css
@@ -15,11 +15,10 @@
   text-align: left; /* LTR */
-  width: 45%;
+  width: 43%;
 }
 [dir="rtl"] .book-pager__item--previous {
   float: right;
@@ -30,11 +29,14 @@

@@ -30,11 +29,14 @@
   width: 8%;
 }
 .book-pager__item--next {
-  float: right; /* LTR */
   text-align: right; /* LTR */
-  width: 45%;
+  width: 43%;
 }

It wouldn't be better 44%?

I tested it with 43% and 44% and both look ok but In my opinion we need to keep the changes at minimium to avoid any unexpected problem.

So, can we change that to 44%?

We are very close, thanks for all your work.

gaurav.pahuja’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
617 bytes

Status: Needs review » Needs work

The last submitted patch, 25: 1340242-25.patch, failed testing.

The last submitted patch, 23: 1340242-23.patch, failed testing.

gnuget’s picture

Alright.

I fixed the failing test in this patch, also I did a manual review in firefox, firefox mobile, chrome, chrome-mobile and safari.

I would like another review before to put this as RTBC .

Thanks all for your work.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Issue summary: View changes

Thank you @gnuget for so many screenshots!

Sadly I cannot judge whether the implementation is done properly :(

+++ b/core/themes/stable/templates/navigation/book-navigation.html.twig
@@ -41,7 +41,7 @@
         <li>
-          <a href="{{ parent_url }}" title="{{ 'Go to parent page'|t }}">{{ 'Up'|t }}</a>
+          <a href="{{ parent_url }}" title="{{ 'Go to parent page'|t }}"><b>{{ '^'|t }}</b> {{ 'Up'|t }}</a>
         </li>

Doesn't stable by construction not change, so should we try to keep it as it is?

gnuget’s picture

Hi Dawehner, thanks for your review.

thank you @gnuget for so many screenshots!

Sadly I cannot judge whether the implementation is done properly :(

I wonder how can help us and judge if this was done properly?

Doesn't stable by construction not change, so should we try to keep it as it is?

Not sure, I can undo that change if necessary.

dawehner’s picture

Someone like davidhernandez would be a great person for that.

davidhernandez’s picture

Status: Needs review » Needs work

how come that there's back & forward arrows but not an up arrow ?

My guess is because of the disruption to the height of the nav bar, pushing that top rule up. *sigh* this is why it would be good to have an empowered design lead, who really should be making the call on things like this. (I'm not saying I disagree with the arrow, just making the point that design changes shouldn't be done in a vacuum.)

+++ b/core/modules/book/templates/book-navigation.html.twig
@@ -43,7 +43,7 @@
+          <a href="{{ parent_url }}" title="{{ 'Go to parent page'|t }}"><b>{{ '^'|t }}</b> {{ 'Up'|t }}</a>

Minor point. I don't think it is necessary for this to go through t is it? I assume that is done for the left and right arrows because the switch from LFT to RTL languages would require the arrow reversing. That shouldn't be needed on an up arrow.

Regarding #30, yes Stable and Classy templates can't be changed. The arrow will have to be left out of those templates, and only added to the core book module one. That leaves a bit of a conundrum. We don't want default CSS in the book module, and I'm not sure we can add CSS in Stable or Classy that is mostly backwards compatible for this and makes sense from a maintenance standpoint. (It would essentially be orphaned CSS without the template.) If we did do that, it at least should have a comment to go with it explaining why it is there and referencing the book module template.

Another option would be to add CSS in Bartik to account for the arrow, if we only care how it looks there from a product standpoint.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

ptmkenny’s picture

I think adding an arrow is confusing for the reasons described in #1503024: Consider changing use of 'Up' as a navigation link in books. Please consider changing the wording "up" as suggested in that issue when addressing this issue as well.

narendra.rajwar27’s picture

Assigned: Unassigned » narendra.rajwar27
narendra.rajwar27’s picture

Assigned: narendra.rajwar27 » Unassigned
cburschka’s picture

I think adding an arrow is confusing for the reasons described in #1503024: Book nav - Wrong wording for "up"?. Please consider changing the wording "up" as suggested in that issue when addressing this issue as well.

Without touching on the issue of wording: I might suggest the following arrow glyph (passed through translation for RTL support)

This one would be harder to mistake for a scrolling direction than ^ or ↑.

mindbet’s picture

The following custom CSS, added to your theme, will add an arrow. Change the unicode to your taste:

.book-pager__item--center a::before {
    content: "\2191 ";
}

If you want to to move the up arrow above the link, start with (and adjust) this:

.book-pager__item--center a::before {
    content: "\2191 ";
    position: relative;
    top: -1.5rem;
    left: 15px;
    line-height: 3rem;
    display: inline-block;
}
.book-pager__item--previous,
.book-pager__item--next {
    line-height: 3rem;
}

sarvjeetsingh’s picture

Status: Needs work » Needs review
FileSize
848 bytes

Made changes as per the suggestions in #45. kindly review

Status: Needs review » Needs work

The last submitted patch, 46: 1340242-46.patch, failed testing. View results

mindbet’s picture

@sarvjeetsingh
The test error is because edits to Classy are not allowed.

see:
core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php

also comment #33 in this issue.

In #45, I was really offering a workaround — people who want an up-arrow can solve it in their theming.

My apologies for implying that it should fixed in book module code.

komalk’s picture

Assigned: Unassigned » komalk
komalk’s picture

Assigned: komalk » Unassigned
Status: Needs work » Needs review
FileSize
5.21 KB
4.58 KB
23.33 KB
22.63 KB

Status: Needs review » Needs work

The last submitted patch, 50: 1340242-50.patch, failed testing. View results

komalk’s picture

Assigned: Unassigned » komalk
komalk’s picture

Assigned: komalk » Unassigned
Status: Needs work » Needs review
FileSize
5.25 KB

fixing test cases.
Refer screenshot attached #50

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

asishsajeev’s picture

Assigned: Unassigned » asishsajeev
asishsajeev’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the patch.
Patch applied successfully. It looks fine to me.

asishsajeev’s picture

Assigned: asishsajeev » Unassigned
catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots, +Needs issue summary update, +Needs usability review, +Needs product manager review

This should be reviewed by the Umami, Olivero, and Claro maintainers as to whether the change should happen in those themes or not. We also need screenshots in each theme to see how it looks, and probably a product manager review for whether it should change the default markup or not.

komalk’s picture

Tested patch #53 on each theme.
Attached the before and after screenshot for reference.

kattekrab’s picture

Issue tags: -Needs screenshots

Screenshots have been added - Issue summary still needs an update.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Postponed

This extension is being deprecated, see #3376070: [Meta] Tasks to deprecate Book module. It will be removed from core and moved to a contrib project, #3376101: [11.x] [Meta] Tasks to remove Book.

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

This issue may be re-opened if it can be considered critical, If unsure, re-open the issue and ask in a comment.