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
Comment | File | Size | Author |
---|---|---|---|
#59 | olivero-before-after.png | 24.04 KB | komalk |
#59 | umami-before.png | 30.25 KB | komalk |
#59 | umami-After.png | 35.19 KB | komalk |
#59 | seven-before.png | 20.57 KB | komalk |
#59 | seven-After.png | 19.76 KB | komalk |
Comments
Comment #1
mgiffordGood 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?Comment #2
kattekrab CreditAttribution: kattekrab commentedThis 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. :)
Comment #3
mgiffordUnassigning stale issue. Hopefully someone else will pursue this.
Comment #5
gnugetComment #6
ancapaaron CreditAttribution: ancapaaron as a volunteer commentedI 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.
Comment #7
gnugetHi ancapaaron
Thanks for your work on this issue.
Can we avoid the use of
<br>
and use styles instead? also, taking ideas from #0 , can we wrap the ^ with<b></b>
?Comment #8
ancapaaron CreditAttribution: ancapaaron as a volunteer commentedHi 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.Comment #9
gnugetThis is looking good! Thanks.
some comments:
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)
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!
Comment #10
Pradnya Pingat CreditAttribution: Pradnya Pingat commentedResolved coding standard issue.
Comment #12
ancapaaron CreditAttribution: ancapaaron as a volunteer commentedHi 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.
Comment #13
ancapaaron CreditAttribution: ancapaaron as a volunteer commentedComment #18
gnugetHi @ancapaaron
This is almost ready, some nitpicks:
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:
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:
Great job so far, thanks!
Comment #19
ancapaaron CreditAttribution: ancapaaron as a volunteer commentedThis 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!
Comment #20
ancapaaron CreditAttribution: ancapaaron as a volunteer commentedComment #22
gnugetOk, some comments here.
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!
Comment #23
gaurav.pahuja CreditAttribution: gaurav.pahuja as a volunteer and at Publicis Sapient commentedComment #24
gnugetIt 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.
Comment #25
gaurav.pahuja CreditAttribution: gaurav.pahuja as a volunteer and at Publicis Sapient commentedComment #28
gnugetAlright.
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.
Comment #30
dawehnerThank you @gnuget for so many screenshots!
Sadly I cannot judge whether the implementation is done properly :(
Doesn't stable by construction not change, so should we try to keep it as it is?
Comment #31
gnugetHi Dawehner, thanks for your review.
I wonder how can help us and judge if this was done properly?
Not sure, I can undo that change if necessary.
Comment #32
dawehnerSomeone like davidhernandez would be a great person for that.
Comment #33
davidhernandezMy 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.)
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.
Comment #41
ptmkenny CreditAttribution: ptmkenny commentedI 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.
Comment #42
narendra.rajwar27Comment #43
narendra.rajwar27Comment #44
cburschkaWithout 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 ↑.
Comment #45
mindbet CreditAttribution: mindbet as a volunteer commentedThe following custom CSS, added to your theme, will add an arrow. Change the unicode to your taste:
If you want to to move the up arrow above the link, start with (and adjust) this:
Comment #46
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedMade changes as per the suggestions in #45. kindly review
Comment #48
mindbet CreditAttribution: mindbet as a volunteer commented@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.
Comment #49
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedComment #50
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedComment #52
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedComment #53
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedfixing test cases.
Refer screenshot attached #50
Comment #55
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedComment #56
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedThanks for the patch.
Patch applied successfully. It looks fine to me.
Comment #57
asishsajeev CreditAttribution: asishsajeev at Zyxware Technologies commentedComment #58
catchThis 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.
Comment #59
komalk CreditAttribution: komalk at Srijan | A Material+ Company for Drupal India Association commentedTested patch #53 on each theme.
Attached the before and after screenshot for reference.
Comment #60
kattekrab CreditAttribution: kattekrab commentedScreenshots have been added - Issue summary still needs an update.
Comment #66
quietone CreditAttribution: quietone at PreviousNext commentedThis 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.
Comment #67
quietone CreditAttribution: quietone at PreviousNext commented