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.
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
feat(react) Support for JSX Widgets in React #9278
base: master
Are you sure you want to change the base?
feat(react) Support for JSX Widgets in React #9278
Changes from 13 commits
9bbdc79
e55ab6f
9803538
c526999
51154e8
e6a7a5f
5549372
b8aaef2
5508ccd
3ed7e27
214f5e6
74c241b
a0d0b6e
26a95a1
271561f
1e62c65
ec8d18f
7cd5d58
b7a79ec
82287da
971da0f
09b577a
7e4c4fb
1e5545f
91c0930
ffe8362
23a3210
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check failure on line 9 in examples/get-started/react/basic/app.jsx
GitHub Actions / test-node
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.
Should we remove the
if (ContextProvider) {
check on L97 if this is always defined now?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'm noticing that the widget's
onViewportChange
isn't called on initialization. The user needs to interact with deck before a viewport is set.Vanilla widgets work. Any idea what's different?
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.
The cause is this guard is hit before the viewports have been assigned to the widget.
lastViewports
was set on the first time props are set (whenwidgets: []
and it initialized the tooltip).The goal of this code is to cache the current viewport so that the widget can use it as the starting point of its view modification.
The hook adds widgets in one-by-one:
[]
then[FirstWidget]
then[FirstWidget, SecondWidget]
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 guess we could push this check down into each widget to implement since the manager isn't aware of how widgets store viewports, or we could add
viewports: {}
toclass Widget
and have the manager check that instead? 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.
See proposed fix in #9303
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.
Wonder if it would be nice to support
defaultProps
for widgets to avoid this sort of codeThere 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.
Widget
is only an interface, so I think the best we can do is lead by example of least annoying code to maintain. I just changed the dev guide to usethis.id = props.id ?? this.id
to avoid needing to repeat the hard-coded values, which is a marked improvement over this.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.
Could it be an abstract class instead?
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.
It's tempting but I'm more in favor of flexibility at this stage. I'm hesitant to abstract too early across widgets with there are so many ways to implement UI.
It's a bit of extra work to fill out the interface, but not much.
Maybe when we have a fuller library of examples exploring a breadth of use cases.. until then, there's nothing preventing an author from making their own abstract class.