uPortal-Project / uPortal-web-components

A collection of uPortal Web Components and JavaScript utilities
https://uportal-project.github.io/uPortal-web-components
Apache License 2.0
24 stars 25 forks source link

Content Carousel: prev/next arrow fonts not loading in uPortal #85

Closed cparaiso closed 6 years ago

fredvanwest-zz commented 6 years ago

I found the prev/next arrows to be there in the favorites carousel but they're extremely hard to find. Had to add more than six favorites and then if I just hit it right, the arrows work. Obviously something could be tweaked to make them easier to see?

ChristianMurphy commented 6 years ago

Issue is specific to browsers with Shadow DOM enabled (Chrome and Firefox with a flag enabled). The font does not get passed through to the Shadow DOM correctly, we may need to update the selector that applies the font.

jgribonvald commented 6 years ago

I think that I had the problem and solved it by adding the font in components too

ChristianMurphy commented 6 years ago

@jgribonvald would you be interested in opening a PR with that patch?

jgribonvald commented 6 years ago

@ChristianMurphy I will try to watch on :wink: But I think this should be like this fix I've done

jgribonvald commented 6 years ago

The only way that I've found is to add

<link href="https://cdnjs.cloudflare.com/ajax/libs/slick-carousel/1.8.1/slick-theme.css" rel="stylesheet">

in html before the content-carousel tag.

jgribonvald commented 6 years ago

I've found that font problems are same than listed here on chrome and firefox nigthly. The table list all working case, and the easiest way is to include the font into the DOM like I've already said.

jgribonvald commented 6 years ago

On an other side there would be good to have a possibility to apply on the next/previous button a different color (a dark class) depending on background color, because depending on the screen and the background, the icons could not be seen.

jgribonvald commented 6 years ago

An other solution to avoid to add the slick font into the dom is to move icon font to Font-Awesome which is already into uPortal, and in this case into ContentCarousel.scss add these lines:

$slick-font-path: "~slick-carousel/slick/fonts/";
$slick-loader-path: "~slick-carousel/slick/";
$slick-font-family:FontAwesome;
$slick-prev-character: "\f053";
$slick-next-character: "\f054";
@import '~slick-carousel/slick/slick.scss';
@import '~slick-carousel/slick/slick-theme.scss';
jgribonvald commented 6 years ago

So after reflexion, if you want to use the slick theme, would be to integrate into dist package the slick-theme.css. Like that it will be possible to import it before the we js into the portlet content. What do you think ? For me it's the good compromise. After is there someone who know how to do that on a pre-build target ?

ChristianMurphy commented 6 years ago

Like that it will be possible to import it before the we js into the portlet content

Not easily, Vue CLI wraps keeps all <style> tags it sees inside the shadow dom. We'd need to do some DOM hacking to add the stylesheet in the header.

For now I'm documenting your suggestion https://github.com/uPortal-contrib/uPortal-web-components/issues/85#issuecomment-432197000 in the README.

ChristianMurphy commented 6 years ago

An other solution to avoid to add the slick font into the dom is to move icon font to Font-Awesome which is already into uPortal, and in this case into ContentCarousel.scss add these lines:

$slick-font-path: "~slick-carousel/slick/fonts/";
$slick-loader-path: "~slick-carousel/slick/";
$slick-font-family:FontAwesome;
$slick-prev-character: "\f053";
$slick-next-character: "\f054";
@import '~slick-carousel/slick/slick.scss';
@import '~slick-carousel/slick/slick-theme.scss';

I tried this, and a couple variants of this, and didn't quite get it to work.

jgribonvald commented 6 years ago

I mean that the package (into dist directory) should have a copy of the slick-theme.css file. Somes do a hack to insert it into the DOM, but in my mind there is lesser cost doing like I suggest as when we publish a content we can add the css file at the same time than the wc js

ChristianMurphy commented 6 years ago

I mean that the package (into dist directory) should have a copy of the slick-theme.css file.

:thinking: that could work What do you see as the advantage of using that over unpgk/cloudflare cdn or adding slick carousel as a webjar to resource server?

jgribonvald commented 6 years ago

A task that make a copy from the dependency is best as it follow directly the same version than used and it avoids to forget to change the css version into links. But that's my point of vue

ChristianMurphy commented 6 years ago

After is there someone who know how to do that on a pre-build target ?

Yep, that would be a simple copy script.

But that's my point of vue

I see what you did there, nice pun :grin:

A task that make a copy from the dependency is best as it follow directly the same version than used and it avoids to forget to change the css version into links.

Okay, that works too. I'll close the current PR and open another with this approach.

ChristianMurphy commented 6 years ago

@jgribonvald https://github.com/uPortal-contrib/uPortal-web-components/pull/147 has been opened with the copy theme approach.