Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(widgets) export package and bundle css in main #8905

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented May 19, 2024

Follow up #8862

Background

Using dist has the advantage of being cleaned by ocular-clean, at the cost of slightly longer URL for scripting users.

https://unpkg.com/@deck.gl/widgets@^9.0.0/dist/stylesheet.css
https://unpkg.com/deck.gl@^9.0.0/dist/widgets/stylesheet.css

The JS import does not use dist in the path:

// using @deck.gl/widgets module
import '@deck.gl/widgets/stylesheet.css';
// using deck.gl module
import 'deck.gl/widgets/stylesheet.css';

Change List

  • include widgets in main JS bundle (import {FullscreenWidget} from 'deck.gl';)
  • include widgets css in main dist
  • fix doc links

Signed-off-by: Chris Gervang <chris@gervang.com>
@coveralls
Copy link

coveralls commented May 19, 2024

Coverage Status

coverage: 89.902% (+0.001%) from 89.901%
when pulling b094d7b on chr/add-widgets-to-bundle
into 987a3e9 on master.

@Pessimistress
Copy link
Collaborator

No problem with including widget exports in main, but I'm not sure about the CSS import path. Is it worth the confusion to save a @ sign?

@chrisgervang
Copy link
Collaborator Author

When using NPM, if deck.gl is the dependency being used in an application, importing anything from @deck.gl/widgets would be confusing, or vis versa.

In the script environment I don't have a strong opinion since they're each a url to the same sheet, however, to implement the NPM version you also get the URLs from each module. We could recommend one and leave the other undocumented?

@Pessimistress
Copy link
Collaborator

Alternatively, in the spirit of the "master" module, we could distribute a combined stylesheet deck.gl/stylesheet.css (currently only contains content from the widgets module).

@chrisgervang
Copy link
Collaborator Author

I like that idea. I'll change it to a top level stylesheet then. We could probably add CSS variables to customize internally defined CSS properties, which would solve some of what #8160 is going after. Variables are only for existing properties though... CSS as JS objects can give you fuller control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants