Align the styling of tool tips to be more consistent with Seven

Sounds great to me! LewisNyman++

@ckrina and me were working on this to make a first approach to resolve the inconsistence of the styles.
Here I attach one patch with an split of the «joyride.css» on two module and theme CSS including little tweaks on properties.
To make a custom mark up to provide a component maybe we should override the definition inside of JavaScript file.
What would the best way to do that? directly Override the jquery.joyride-2.0.3.js file or implement a cleaner / independent override on tour.js

Note that if you want to align CSS more with the Seven style guide, then the CSS should live in the Seven theme's CSS. Therefor Seven-aligned CSS in tour.module.css does not seem acceptable.

@ckrina and me were working on that again, here I attach new patch following indications.
We moved the styles inside a new Seven component.

Thanks a lot for the review. Here I attach new patch following your indications, I hope be correct now.

#2152377: Failed to change the status to 'needs work' after test failure

11: tour-styling_tooltips_tour-1921044-11.patch queued for re-testing.

Seems that is necessary modify the test "Drupal\Tests\Core\Extension\ThemeHandlerTest" to include the new css files proposed in this issue in Seven theme, but I don't know if this change can do from this issue...

@ckrina and me attached the patch again including the test modified.

Seems like the test which failed (GlossaryTest) it doesn't concern about this issue, we modified the "ThemeHandlerTest" and now this passed... I'll launch the test again...

18: tour-styling_tooltips_tour-1921044-18.patch queued for re-testing.

Ok in general! I did the two tweaks in few minutes and I have a new patch ready.
Although about the last comment I think to allow put minus margin at only "left" and "right" tooltips (which are misaligned) we would need a class in the general wrapper of the tip that marked the kind of tip. But now the TipLocation variable only is puts a classname on joyride-nub div inside of the general wrapper... and it doesn't work
I've had a look at the tour.js but I don't see how to do that... I'll try to further investigate but all the support are wellcome.

Hi pakmanlh!

Text tips have a 'location' property assigned to them. This set the top/left/right/bottom positions. We could use that to provide a class to the wrapper of the tip.

Would that help unblock this issue for you?

Yes, but the question is "How" to do that :P
Anyway thanks for the feedback!

Ok! Here I attach a new patch that fix the last comments ignoring the vertical misaligned.

Visually tested agains HEAD in Seven and Stark. Looks good, except for the alignment issue mentioned before. Actually, this only seems to be an issue when the Joyride tip is pointing horizontally at an item, and could be resolved by applying the background/border/radius to the inner content area and shifting it up/down.

I re-rolled the patch again.

jquery.joyride:
  remote: https://github.com/zurb/joyride
  version: v2.0.3
  js:
    js/jquery.joyride-2.0.3.js: {}
  css:
    component:
      css/joyride-2.0.3.css: { media: screen }
  dependencies:
    - core/jquery
    - core/jquery.cookie

This refers to the removed CSS file?

Plus we're breaking up the external library - is that really the done thing? I guess this is okay

.joyride-tip-guide .joyride-nub.top {
  top: -28px;
  bottom: none;
}

.joyride-tip-guide .joyride-nub.right {
  top: 22px;
  bottom: none;
  left: auto;
  right: -28px;
}

.joyride-tip-guide .joyride-nub.left {
  top: 22px;
  left: -28px;
  right: auto;
  bottom: none;
}

W3C: none is not a bottom value

.toolbar .tour-toolbar-tab button.active {
  background-image: -webkit-linear-gradient(rgba(255, 255, 255, 0.25) 20%, transparent 200%);
  background-image: linear-gradient(rgba(255, 255, 255, 0.25) 20%, transparent 200%);
}

Why only webkit? http://www.w3schools.com/css/css3_gradients.asp

+++ b/core/modules/tour/js/tour.js
@@ -102,7 +102,11 @@
+           },

jshint says that this is wrongly indented and PHPStorm complains about the comma :)

I rerolled the patch with the Alex indications (thanks a lot!)
Only that I'm not sure if it would necessary remove the css file reference at the component definition.

Not sure why it fails testing but it looks awesome when I test it on simplytest.me.

Only issue I see is unrelated to this one, the arrows not quite pointing at the right thing.

The test fails because it try to remove joyride.css that it has been moved to new path... I see that last friday but I had not time yet to solve, because maybe this fact implies modify component and some test... Feel free to reroll, I'll try to do as soon as possible :P

Great to see tour module being aligned to the style guide - this looks a lot better!

I've found it confusing to mix em based & pixel based units for defining the padding of the tooltip container, see .tour-progress, .joyride-content-wrapper and .joyride-close-tip.

In the attached patch iteration, i tried to fix the problem of repeating padding units of the parent .joyride-content-wrapper by moving its padding up to the .joyride-tip-guide. This way we can just use right: 0; to declare the children elements to be positioned at some corner instead of formerly mixing pixels and ems.

+++ b/core/modules/tour/css/tour.module.css
@@ -18,29 +18,138 @@
+    width: 95%;

This doesn't look good on mobile (or smaller screens):

I created a patch that changes the width to 87%, which is a kinda weird number ... but it fixes the problem:

Another thing to do, might be to completely remove the width attribute. This way - the tour popups don't fill up the entire screen, yet it still looks good.

Looks nice! Some minor clean-up and ready to go (I hope).

This patch looks fine. This makes the tour popup looks good for all breakpoints. Nice!

Works fine for me too

@Lewis I wonder if this should have more round corners.

The reference in the .yml file it was removed, so I think is all ready :)

48: tour-styling_tooltips_tour-1921044-48.patch queued for re-testing.

FYI, my concerns are resolved its only in mobile that we lose the corners because it would otherwise cramp the textarea too much

rock & re-roll!

No review, no rock :P

58: tour-styling_tooltips_tour-1921044-58.patch queued for re-testing.

Great chatch Lewis!
No clue how that happened, here it is again without that plus sign.

sorry forgot to add it before making the patch... here it is again

rerolling.

This is just a re-roll of #68.

$ git apply -v 19210044-tour-styling_tooltips_tour-73.patch
Checking patch core/assets/vendor/jquery-joyride/joyride-2.0.3.css...
Checking patch core/core.libraries.yml...
Checking patch core/modules/tour/css/tour.module.css...
Checking patch core/modules/tour/js/tour.js...
Checking patch core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php...
Checking patch core/themes/seven/css/components/tour.theme.css...
Checking patch core/themes/seven/seven.info.yml...
Applied patch core/assets/vendor/jquery-joyride/joyride-2.0.3.css cleanly.
Applied patch core/core.libraries.yml cleanly.
Applied patch core/modules/tour/css/tour.module.css cleanly.
Applied patch core/modules/tour/js/tour.js cleanly.
Applied patch core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php cleanly.
Applied patch core/themes/seven/css/components/tour.theme.css cleanly.
Applied patch core/themes/seven/seven.info.yml cleanly.

Checked the content view tour.. looks fine =)

73: 19210044-tour-styling_tooltips_tour-73.patch queued for re-testing.

After tested in my local environment I launched the test again and now the test passed :)

Committed 9b87b2b and pushed to 8.x. Thanks!

Is there any reason for removing the CSS file? It seems removing it from the libraries definition should be enough, right? That way we wouldn't have to hack an external library.

1. index ba103f0..0000000
   --- a/core/assets/vendor/jquery-joyride/joyride-2.0.3.css
   

   This file shouldn't have been removed.

2. +++ b/core/core.libraries.yml
   @@ -331,9 +331,6 @@ jquery.joyride:
   -  css:
   -    component:
   -      assets/vendor/jquery-joyride/joyride-2.0.3.css: { media: screen }
   
   +++ b/core/themes/seven/seven.info.yml
   @@ -10,6 +10,7 @@ stylesheets:
   +    - css/components/tour.theme.css
    stylesheets-override:
   

   Why didn't you use the stylesheets-override facility in seven's .info.yml file to override the default joyride stylesheet?

3. +++ b/core/modules/tour/css/tour.module.css
   @@ -18,29 +18,132 @@
   +.toolbar .tour-toolbar-tab.toolbar-tab.hidden {
   

   It doesn't look like the Tour module CSS changes were tested in any other core theme?

4. +++ b/core/modules/tour/css/tour.module.css
   @@ -18,29 +18,132 @@
   +/* JoyRide Styles. */
   

   This appears to be a copy of the original/vendor jQuery Joyride styles...?

   Why were the original styles deleted then?