-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
add(widgets) fullscreen widget #8024
Conversation
Please leave design comments in our prototype. Open to suggestions! |
Learning a bit more about CSS variables. They aren't supported inside media queries directly, but a combination of JS and CSS can be used. See discussion. It'd be ideal to keep variable names shorter, such as Supporting browsers that lack css variable support is straightforward. Just define each style twice with and without a variable, and the cascading will apply the latter "valid" property. |
@@ -57,5 +58,6 @@ new Deck({ | |||
getTargetColor: [200, 0, 80], | |||
getWidth: 1 | |||
}) | |||
] | |||
], | |||
_widgets: [new FullscreenWidget()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assume you will create a new example for effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pessimistress suggested I add to the getting started, but I'm not sure if this is what she meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a separate example unless you think it's a good functionality to surface on all the examples. Each get-started flavor (pure-js, react, script) demonstrate exactly the same functionality.
Co-authored-by: Ib Green <ib@foursquare.com>
Co-authored-by: Ib Green <ib@foursquare.com>
fc6be2c
to
f783461
Compare
b00ae70
to
46be41a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts:
-
Do we have provisions for declaratively setting props on this component (let's say that the component offered a
hideNavigationUI
prop and the app wanted to change that after creating the component. -
What if the app programmatically wants to active the effect that an interactive widget controls. Let's say that the app wanted to trigger full screen, how would it do that?
-
Docs - In documentation mention that this feature is not available on iPhones?
https://caniuse.com/?search=requestFullscreen -
Tests - testing the fullscreen effect may be overkill, but at least add the control to the DOM and and remove it when testing under browser?
Evaluated RE:DOM as a lightweight html-in-js solution that is supposed to offer a JSX syntax. JSX works fine without typescript in this isolated example I made, but I'm not figuring out how to get redom JSX to work with typescript in this other example. RE:DOM and typescript don't seem to be pretty commonly documented together. And babel seems to have moved on to a new way to building jsx plugins, which RE:DOM doesn't implement. Resouces
Setup notes: // babel.config.js
...
plugins: [
"babel-plugin-transform-redom-jsx",
[
"transform-react-jsx",
{
"pragma": "el"
}
]
] // tsconfig.json
"compilerOptions": {
"jsx": "preserve",
"jsxFactory": "el"
...
}, |
@@ -30,7 +30,8 @@ | |||
"prepublishOnly": "npm run build-bundle && npm run build-bundle -- --env=dev" | |||
}, | |||
"dependencies": { | |||
"@babel/runtime": "^7.0.0" | |||
"@babel/runtime": "^7.0.0", | |||
"preact": "^10.17.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, I haven't really being paying attention. Are we using a react style library for our pure-js implementation? And then we will have react wrappers on top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty certain we don't want the complexity or bundle size of react, but I like its api style.
My goal has been to pick something as close to the DOM as we can while also providing some convenience over document.createElement
. preact
seems to serve this niche well... It give us the ability to make basic components with JSX, and implements a thin virtual dom only adding 3kb to the bundle.
Re: react wrappers, I think of a widget as a deck component that happens to have UI. In a sense, how the UI is built within it is ideally isolated from other UI on the page so that deck can manage it. Let's say we used react to make UI widgets in @deckgl/widgets
... I'd still ideally want them to create their own react root separate from the rest of application and other widgets. This way all integrations work through the same interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and I agree with the principle that widgets should be free to choose implementation as long as they are externally independent. That said, I would not be happy to see a pure-js deck.gl module have react as a dependency in its package.json. I would think twice before using that in a pure-js app.
For a community widget that could be a different story of course.
preact seems like a reasonable choice.
const el = document.createElement('div'); | ||
el.classList.add('deckgl-widget', 'deckgl-widget-fullscreen'); | ||
if (className) el.classList.add(className); | ||
Object.entries(style).map(([key, value]) => el.style.setProperty(key, value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how Deck applies style: https://github.com/visgl/deck.gl/blob/master/modules/core/src/lib/deck.ts#L734
I have not tested it but the two approaches seem equivalent?
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty#alternative_usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new FullscreenWidget({}), | ||
new FullscreenWidget({id: 'themed', style: widgetTheme}), | ||
new FullscreenWidget({id: 'purple', className: 'purple'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pessimistress I was thinking of move these variations to a test app. I want to maintain coverage for edge cases somewhere, and keep getting started very simple. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes agreed
Users may want to replace the icons, so I've moved them to CSS. To replace with your own we'll document: Icons are replaceable and their colors can be customized with CSS variables.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pessimistress, added sections for replacing icons and made other adjustments. Any comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @ilyabo, @felixpalmer, @ibgreen
For #7946
Background
A fullscreen widget adds a button which users can click to make their map full screen. Click it again to exit full screen.
Design Goals
themes.ts
provides 4 built-in themes, and the getting started example shows how these can be applied.With CSS variables it's possible for applications to make customizations, such as:
prefers-color-scheme
media-query, or prop override)max-width
media-query, or prop override)Implementation Goals
preact
UI framework internally to minimize complexity and bundle size.Compatibility
Currently the widget assumes the Fullscreen API is supported without prefixes since it's a mature API at this point. If it's not, then a "pseudo" fullscreen is styled with css.
Change List