Add injectStyles, so Dropzone can supply its own CSS - #2363
Open
enyo wants to merge 1 commit into
Open
Conversation
Contributor
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
Setting injectStyles makes Dropzone insert dist/dropzone.css into the document itself, so there is no link tag to remember and no path to keep in step with the package. The stylesheet is carried as a string rather than imported for its side effect, so nothing reaches the document unless the option asks for it. It is inserted once per page however many dropzones exist, and prepended to head rather than appended. Appending would put it after a stylesheet the page already links, so switching the option on would quietly override styling that used to work. Prepending means page rules keep winning on equal specificity, with no need for !important. It runs before the fallback check, because the fallback form is styled by the same stylesheet. Defaults to false, and this is the tradeoff worth stating plainly: the CSS travels inside the JavaScript bundle whether or not the option is on, because a runtime condition cannot be tree-shaken. That is 4.9 kB minified, 1.3 kB gzipped, on a bundle that was 11.8 kB gzipped. Anyone importing the CSS through a bundler should keep doing that; this is for the people who would otherwise ship no stylesheet at all. The end-to-end test loads a page with no stylesheet link whatsoever and asserts a computed min-height of 150px, so it proves the CSS survived bundling into the standalone file rather than merely that a style element appeared.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #2362. Setting
injectStylesmakes Dropzone insertdropzone.cssinto the document itself — no<link>to remember, no path to keep in step with the package.The CSS is carried as a string rather than imported for its side effect, so nothing reaches the document unless the option asks for it.
Two decisions worth reviewing
It is prepended to
<head>, not appended. Appending would place it after a stylesheet the page already links, so turning the option on would quietly override styling that used to work. Prepending means your rules keep winning on equal specificity, with no!important. There is a test for the ordering, because it is the kind of thing a later refactor silently reverses.It runs before the fallback check, since the fallback form is styled by the same stylesheet.
The cost, stated plainly
injectStylesdefaults tofalse, and this is why: the stylesheet travels inside the JavaScript bundle whether or not the option is switched on, because a runtime condition cannot be tree-shaken.dropzone-min.jsThat is +1.3 kB gzipped, about 11%, paid by everyone. Anyone importing the CSS through a bundler should keep doing that — it stays the smaller option, and the docs say so. This is for the people who would otherwise ship no stylesheet at all.
If that cost turns out to bother you, the alternative is a separate entry point (
dropzone/with-styles) that only its importers pay for — but that is an import, not an option, which is not what you asked for.Tests
7 unit tests and 2 end-to-end. The end-to-end ones matter more than usual: the page they load links no stylesheet at all, and the assertion is a computed
min-heightof150px. That proves the CSS survived bundling into the standalone file and is actually applying — not merely that a<style>element appeared.259 unit tests and 5 end-to-end specs pass overall. The docs are updated in both places: a row in the options table, and a section under Installation → CSS that includes the size tradeoff.
A fix that belongs to #2362
While building this I found #2362's CI failing on
format:checkforsrc/dropzone.css. Cause:oxfmtruns with--ignore-path .gitignore, and the stalesrc/.gitignoreI deleted in that pull request had been hiding the file from the formatter as well as from git. Fixed on that branch, not this one, and this branch was rebased onto it.