fancyapps / ui

A library of JavaScript UI components, includes the best lightbox - Fancybox5
https://fancyapps.com/
Other
822 stars 99 forks source link

[Suggestion] Class names namespace to avoid conflicts #87

Closed AndersMad closed 1 year ago

AndersMad commented 3 years ago

Problem: Class name carousel is too generic - e.g. conflicts with bootstrap or prior own etc.

Solution: Add a fb namespace: e.g. fb- so it becomes fb-carousel__button

Sub problem: The options can change the class name - but e.g. carousel__button seems to be "hardcoded" in Fancybox.js

Eldzej commented 3 years ago

You already can override all the classnames, it just takes a bit of a work.

In the options, you can do the following

classNames: {
    viewport: "carousel__viewport",
    track: "carousel__track",
    slide: "carousel__slide",
    slideSelected: "is-selected",
},

And alter as needed. You need to manually alter the styles after you do this though

Though it could be made a bit more clear, or maybe just set it in one place (some kind of prefix option maybe, that would get copied across the board?)

Agreed on better prefix for Carousel, that is rather generic, as opposed to Fancybox and Panzoom. Maybe something like fancyapps_carousel, fancyapps_fancybox, etc?

fancyapps commented 3 years ago

Hi,

I like the idea about the prefix, I think I will add that as an option.

AndersMad commented 3 years ago

I was more thinking of a generic prefix for all classes, could be fancyapps - but fancyapps_fancybox__slide sounds a bit... verbose :) But a prefix for the most generic would ok too / good enough I spose 👍

I just often find that class names quickly becomes to generic. Targeting the styles can be ok easy like wrap the whole SCSS import in a main class (although a bit bloated) - but that does not help if e.g. bootstrap (and its a bs discussion too I have seen) "bleeds out" across. And also a query selector like ".carousel" will end up "getting too may". I am in an environment where I have no idea what else is there - even in the SCSS stack.

Eldzej commented 3 years ago

Agreed that is a bit verbose, I was thinking about something that would not conflict with other stuff (just fa sounds like related to FontAwesome, etc.)

AndersMad commented 3 years ago

Ye, and FontAwesome actually have this SCSS:

$fa-css-prefix: crazyprefix;
.#{$fa-css-prefix}-spin { ...

So if an option - it would go into the SCSS + the JS options (must be same ofc.). Else just go with e.g. fap (fancy app :D )

fancyapps commented 3 years ago

Honestly, naming was a very difficult decision. All common names are already taken (like Swiper), a random name would also confuse or would be hard to remember, fa- (or f-) prefix could be confused with Facebook or other projects.

On the other hand, Carousel needs very little CSS for basic functionality. You could easily set your custom names using JS options and then just write your CSS that you need.

AndersMad commented 3 years ago

Naming always is ain't it! ;-) Well, as I am test running your v4 here - my decision is to base it on top of your SCSS files - cause just in case you add some sort of really important thing like css transform, overflow, grid/flex swap etc. that is crucial - then the JS and SCSS package update will take care of that..

jmalatia commented 2 years ago

+1 for ALL class names used in this project to be namespaced (e.g. fb4-*) by default (or main classname namespaced and all other classes nested under it in scss/css).

Too many other libraries in use now-a-days to use non-namespaced class names. It you are worried about classname length, I would rather see the namespace and a less verbose classname.

Fancybox V4 class names should also be isolated from Fancybox V3 classnames for those migrating to avoid conflicts. Not sure if they are or not--- haven't jumped in that far yet.

.carousel, .compensate-for-scroll are common classnames.

fancyapps commented 2 years ago

I am going to change naming in v5. Which option would you prefer? Maybe there are other suggestions?

.FancyPanzoom {}        .FancyPanzoom__content {}
.FancyCarousel {}       .FancyCarousel__slide {}
.FancyBox {}            .FancyBox__content {}

OR

.fancy-panzoom {}        .fancy-panzoom__content {}
.fancy-carousel {}       .fancy-carousel__slide {}
.fancybox__container{}   .fancybox__content {}
AndersMad commented 2 years ago

Personally the latter.

And here is another suggestion - a bit shorter, split of fancybox and with BEM -- for "state" (A):

.fancy_panzoom {}         .fancy_panzoom-content {}
.fancy_carousel {}        .fancy_carousel-slide {}    .fancy_carousel-slide--active {}
.fancy_box-container{}    .fancy_box-content {}

Or even shorter (B):

.f_panz {}          .f_panz-content {}
.f_caro {}          .f_caro-slide {}    .f_caro-slide--active {}
.f_box-container{}  .f_box-content {}

But anything goes :)

Eldzej commented 2 years ago

Personally I prefer snake_case and/or kebab-case for naming stuff in projects over CamelCase

Maybe slight suggestion for sake of clarity, use fancy as prefix always, including Fancybox (so something like fancy_box, fancy_carousel etc, if going with 🐍).

I like the first suggestion from @AndersMad, the second would probably run into problems of being too confusing with other libraries, as I mentioned before.

fancyapps commented 2 years ago

Thanks for the suggestions. I will probably choose the simplest solution to add a prefix to all class names. Not sure which one to choose, maybe f- would be fine - simple and short:

.f-panzoom {}   
.f-panzoom__viewport {} 
.f-panzoom__content {}

.f-carousel {}
.f-carousel__track {}
.f-carousel__slide {}

.f-fancybox__container {}
.f-fancybox__backdrop {}
.f-fancybox__slide {}
.f-fancybox__content {}
.f-fancybox__caption {}
fancyapps commented 1 year ago

Hi,

So, v5 has been released recently and class names are prefixed with f- except for Fancybox.

ryanburnette commented 1 year ago

You might want to revisit this. Many of the fancybox classes are not scoped such as is-hidden. It would be helpful if you would scope everything as to not come into conflict with existing frameworks.