fix: move reveal-centered into foundation-reveal mixin

[DanielRuf]

No description provided.

[ncoden]

Let's continue the discussion in the right PR:

At least it makes not much sense to add it up there. The other code in my PR works too. Deduplication is done by libsass.

-- #11086 (comment)

Yes it makes sense, but it breaks reveal-modal-width. That's what I was explaining in #11086 (comment). The placeholder is used in reveal-modal-width, and that mixin should be usable without having to call foundation-reveal first.

Again I don't understand: did you test it ? Does it generate code when importing Foundation without calling any mixin ? I did that for testing for a long (for example to test the breakpoint mixin) and never seen that piece of code being generated.

[DanielRuf]

Does it generate code when importing Foundation

Sure, if you mean https://github.com/zurb/foundation-sites/blob/develop/dist/css/foundation.css#L8

[DanielRuf]

The placeholder is used in reveal-modal-width

Then it should be only in reveal-modal-width I guess.

[ncoden]

Does it generate code when importing Foundation without calling any mixin.

@import "foundation-sites';
// and that's all

[DanielRuf]

Does it generate code when importing Foundation without calling any mixin.

The following does add the code at the top:

@import 'settings/settings';
@import 'foundation';
@import 'motion-ui';

@include foundation-everything($flex: false);

[DanielRuf]

Without calling anything else it is not added.

[DanielRuf]

But imo the current way how it works (added at the top) is not a very clean solution as it pollutes the statements at the top and changes the order of meaning (between normalize and the foundation-sites header) a bit.

[DanielRuf]

Let's hear from @kball and @SassNinja what they think about the current code and my proposed solution.

So far imo the code should be at the right position to keep the context.

[ncoden]

Does it make more sense to have this piece of code next to the Reveal code

Yes, obviously. Not because we would like to have the generated code more beautiful (we don't care, it will be minified anyway) but because CSS rules generated first are overriden by the next ones.

Can we make this being generated at the right place

As said previously, we can't without introducing a bug. As %reveal-centered is used in @mixin reveal-modal-width, moving it to @mixin foundation-reveal prevent to use @mixin reveal-modal-width without @mixin foundation-reveal being called before. Because Foundation is also a Sass library, all mixins should being usable (and generate their own code) without the "main components mixins" @mixin foundation-* or @mixin foundation-everything.

The only solution is to move it to a dedicated mixin that must be called first but does not generate anything. That's an initialization step, and the current Foundation Sass architecture does not have it (only configuration, import and execution).

Is this code being generated at the wrong place a bug (or shoud it be resolved)

It would if it introduced a specificity issue, or if code was generated at the import step (this is what I understood from #11086 (comment)). Actually it does not that. So it's not a bug, just not perfect and a bit ugly in the distribution files. So it's a much too minor issue to change the Sass architecture for it (but we can keep it in mind for v7).

What do you think ?

[DanielRuf]

The only solution is to move it to a dedicated mixin that must be called first but does not generate anything.

That's what would have been my next question and step: Shouldn't we isolate this in a separate mixin?

[ncoden]

That's what would have been my next question and step: Shouldn't we isolate this in a separate mixin?

Yes, something like @mixin foundation-init-reveal, but that mean we should expect the user to call it (like in foundation.scss or as shown it the Foundation Sass documentation) to be able to use reveal. So for now I'm for keeping that for later. That's only a minor issue without any bugs, let's not requiere migrations for that.

[DanielRuf]

Yes, something like @mixin foundation-init-reveal, but that mean we should expect the user to call it (like in foundation.scss or as shown it the Foundation Sass documentation) to be able to use reveal. So for now I'm for keeping that for later. That's only a minor issue without any bugs, let's not requiere migrations for that.

Agreed.

[ncoden]

Introduce a bug when reveal-modal-width is called without foundation-reveal.

[ncoden]

See #10101

[DanielRuf]

I saw it already but we still need to wrap this in a mixin imho.

[ncoden]

Agreed but #11087 (comment) ;)