added automatically loading the default esri's css stylesheet #80

SimplyAutomationized · 2018-01-19T02:43:26Z

found useful to load the default css stylesheet automatically

tomwayson

I don't know if you saw the open issue that we have for this: #6

What you've implemented in this PR is close to what we laid out in that issue as the first iteration of this feature, so I'd like to merge this. However, there are few things that will need to happen before we could release this.

The main change is that I don't think we should load CSS by default. I'd guess that most people actually would like that, especially b/c in this case it's not like the esri styles are available in some other format like Sass that users might want to bring into their app via some other asset pipeline. However, in the even that someone wants to load the styles by some other means, I don't think the API you've created in this PR will allow them to not inject a style <link>. Also, injecting styles by default would be a breaking change and require a major version bump.

I've made some line comments suggesting changes to make injecting the styles completely optional. In addition to those, we would need to

document how to use this in the README
write additional tests that verify that the style tag is not injected by default and is injected when you pass the css option
update the Unreleased section of the changelog

Please let me know how much, if any, of that you are up for and either myself or @jwasilgeo will be available to help.

Thanks again for your contribution!

tomwayson · 2018-01-19T04:04:46Z

src/esri-loader.ts

@@ -12,7 +12,7 @@
 */
 const isBrowser = typeof window !== 'undefined';
 const DEFAULT_URL = 'https://js.arcgis.com/4.6/';
-
+const DEFAULT_CSS = 'https://js.arcgis.com/4.6/esri/css/main.css';


Remove this, it won't be needed since we're not loading styles by default.

tomwayson · 2018-01-19T04:06:42Z

src/esri-loader.ts

@@ -72,7 +80,9 @@ export const utils = {
 export function getScript() {
  return document.querySelector('script[data-esri-loader]') as HTMLScriptElement;
 }
-
+export function getCss() {
+  return document.querySelector('style[data-esri-loader]') as HTMLStyleElement;


I think this would actually be a HTMLLinkElement

tomwayson · 2018-01-19T04:11:19Z

src/esri-loader.ts

-    options.url = DEFAULT_URL;
+      options.url = DEFAULT_URL;
+  }
+  if (!options.css) {


Remove this, we only want to load styles if the options.css was passed in.

tomwayson · 2018-01-19T04:12:00Z

src/esri-loader.ts

  }

  return new utils.Promise((resolve, reject) => {
    let script = getScript();
+    let cssLib = getCss();


this is not used, it probably should be removed.

tomwayson · 2018-01-19T04:14:58Z

src/esri-loader.ts

@@ -119,6 +133,7 @@ export function loadScript(options: ILoadScriptOptions = {}): Promise<HTMLScript
        }
        // create a script object whose source points to the API
        script = createScript(options.url);
+        cssLib = createCssLib(options.css);


this line and the document.body.appendChild(cssLib); line below should only be executed if options.css was passed in. So:

if (!options.css) { document.body.appendChild(createCssLib(options.css)); }

tomwayson · 2018-01-19T04:21:21Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "esri-loader",
-  "version": "2.0.0",
+  "version": "2.1.0",


This will need to be reverted - the it will be handled by our release script. Generally it's a good idea to leave version bumps out of PRs.

tomwayson · 2018-01-19T05:13:03Z

After thinking about it, if we really think that most people will want loadScript() to lazily inject the <link> tag when it injects the <script> tag, we should probably do that by default rather than make people pass the url to the css - even if that's a breaking change.

We'd still have to change the API defined in this PR so that it allows people load the styles by other means. I haven't put much thought in this yet, but I'm not a big fan of css: false b/c I don't like explicitly checking for false, but I don't have a better idea yet. I'm open to suggestion.

jwasilgeo · 2018-01-19T20:07:40Z

Thanks @SimplyAutomationized for sharing your contribution.

I agree with the code review and PR housekeeping requests above, @tomwayson. I began to write too much here, so I'm going to move my comments over to issue thread #6.

- reverted version - added changes changelog [Unreleased] - check if the stylesheet has been added manually.

SimplyAutomationized · 2018-01-19T21:37:40Z

after your second thoughts do you still want to remove
const DEFAULT_CSS = 'https://js.arcgis.com/4.6/esri/css/main.css';
I added some checks with the getCss(url) method it checks for any link elements containing url or 'esri/themes/' or the url.

export function getCss(url) {
  return document.querySelector(`link[href*="${url}"], link[href*="/esri/themes/"`) as HTMLLinkElement;
}

thoughts?

tomwayson · 2018-01-24T07:07:55Z

@SimplyAutomationized thanks for following up w/ the changes. They look good.

I still haven't had time to think about what we'll eventually want the API to be for this.

In the mean time, I'd like to land your commits into incremental, non-breaking changes so that people can start benefiting from them sooner than later and we can hash out an API over time.

I haven't put much thought into this, but it seems like the minimally viable, non-breaking API would be to export a new function called loadCss(url) that would call the code you've added to load the <link> tag. That should be fairly easy to release (i.e. w/ tests and documentation).

From there we can think about how to be smart about how we can automagically ✨ call that new function from loadScript()/loadModules() function and how to default to the right url based on the url of script tag, etc.

I'm pretty swamped right now and don't have time to give more direction that that. If you or @jwasilgeo feel like that makes sense and can run w/ that, great. Otherwise, I'll make a new branch based on yours and give it a go when I get a chance.

jwasilgeo · 2018-01-24T21:51:59Z

Seems like a good plan to me.

...it seems like the minimally viable, non-breaking API would be to export a new function called loadCss(url) that would call the code you've added to load the tag. That should be fairly easy to release (i.e. w/ tests and documentation).

How does that sound to you, @SimplyAutomationized? I can help out.

added loading the default css stylesheet automatically

f05a0d2

tomwayson requested changes Jan 19, 2018

View reviewed changes

jwasilgeo mentioned this pull request Jan 19, 2018

loadScript has option to also create JSAPI css link tag(s) #6

Closed

- misc. housekeeping

c99974e

- reverted version - added changes changelog [Unreleased] - check if the stylesheet has been added manually.

tomwayson mentioned this pull request Feb 5, 2018

Feat/load css #83

Merged

tomwayson merged commit c99974e into Esri:master Feb 7, 2018

tomwayson mentioned this pull request Feb 7, 2018

loadScript() takes a css option to load stylesheet by URL #86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added automatically loading the default esri's css stylesheet #80

added automatically loading the default esri's css stylesheet #80

SimplyAutomationized commented Jan 19, 2018

tomwayson left a comment

tomwayson Jan 19, 2018

tomwayson Jan 19, 2018

tomwayson Jan 19, 2018

tomwayson Jan 19, 2018

tomwayson Jan 19, 2018

tomwayson Jan 19, 2018 •

edited

tomwayson commented Jan 19, 2018

jwasilgeo commented Jan 19, 2018

SimplyAutomationized commented Jan 19, 2018

tomwayson commented Jan 24, 2018 •

edited

jwasilgeo commented Jan 24, 2018 •

edited

added automatically loading the default esri's css stylesheet #80

added automatically loading the default esri's css stylesheet #80

Conversation

SimplyAutomationized commented Jan 19, 2018

tomwayson left a comment

Choose a reason for hiding this comment

tomwayson Jan 19, 2018

Choose a reason for hiding this comment

tomwayson Jan 19, 2018

Choose a reason for hiding this comment

tomwayson Jan 19, 2018

Choose a reason for hiding this comment

tomwayson Jan 19, 2018

Choose a reason for hiding this comment

tomwayson Jan 19, 2018

Choose a reason for hiding this comment

tomwayson Jan 19, 2018 • edited

Choose a reason for hiding this comment

tomwayson commented Jan 19, 2018

jwasilgeo commented Jan 19, 2018

SimplyAutomationized commented Jan 19, 2018

tomwayson commented Jan 24, 2018 • edited

jwasilgeo commented Jan 24, 2018 • edited

tomwayson Jan 19, 2018 •

edited

tomwayson commented Jan 24, 2018 •

edited

jwasilgeo commented Jan 24, 2018 •

edited