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

Geomap: add initial openlayers alpha panel #36188

Merged
merged 83 commits into from
Jul 9, 2021
Merged

Geomap: add initial openlayers alpha panel #36188

merged 83 commits into from
Jul 9, 2021

Conversation

ryantxu
Copy link
Member

@ryantxu ryantxu commented Jun 25, 2021

See also: #36585

This is an alternative approach from:
#35915

Rather than using leaflet, it is based on open layers. Open layers is way more feature packed, but does require thinking about projections.
Peek 2021-06-25 17-41

TODO:

  • CSS > SCSS? (needs real class names)
  • Layer configuration / UI?
  • Panel view options
  • Data > Layer options

const category = getOptionsPaneCategory(pluginOption.category);
const Editor = pluginOption.editor;

// TODO? can some options recursivly call: fillOptionsPaneItems?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torkelo -- I refactored so LayerEditor uses the same function. We could explore getting nested properties from the plugin. For example maybe:

pluginOption.getNestedOptions(context.options, context.data) => PanelOptionsEditorItem[]

and then we could recursively call fillOptionsPaneItems?

I think it could work -- but should be pursued in its own issue

name?: string; // configured display name

// Layer transparency
transparency?: number;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could leave this out now and add it when it is used -- the point is to make sure we have a level of options that apply to all layers regardless of type. Other values will include:
https://openlayers.org/en/latest/apidoc/module-ol_layer_Base-BaseLayer.html

Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryantxu I've tested and went through the code and it looks impressive! I have doubt about the nested PanelOptionsUIBuilder, but other that that the rest of my comments are cosmetic at this stage

Comment on lines 48 to 50
constructor(props: Props) {
super(props);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

}

export class DebugOverlay extends PureComponent<Props, State> {
style = getStyles(config.theme);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We keep styles in the render function- it's memoized so is not a perf issue

export const LayerEditor: FC<LayerEditorProps> = ({ config, onChange, data, filter }) => {
// all basemaps
const layerTypes = useMemo(() => {
return geomapLayerRegistry.selectOptions(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking along the same lines as @torkel. I would expect either using showIf for given layer editor rendering, or passing down the original builder into custom editor so that the extended options could become subject of search too :) I think the problem will be with changing editors, as we do not re-initialize the builder every time and we would have to have a way to remove the options when the custom editor unmounts.

Comment on lines +28 to +36
<Select
options={views.options}
value={views.current}
onChange={(v) => {
onChange({
id: v.value!,
});
}}
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this editor be created with just addSelect instead? And then the coordinates editor would become a conditional option(s)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe -- but I was hoping for something a little more fancy that knows what the current map view is and can explicitly get them in sync.

context,
}) => {
// TODO:
// Somehow use context to get the current map and listen to zoom changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not hooking up the panel.onPanelOptionsChange callback to zoom interaction directly in the panel implementation? I don't think options pane should ever reference the implemetation detail of the panel

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it should be explicitly set from the editor -- In the editor I want two things:

  • show your current zoom/view
  • press a button to use it

We can (and likely should) send the current view over the EventBus -- this is fine for getting changes, but you can not get the current value :(

id: 'view.zoom',
path: 'view.zoom',
name: 'Initial zoom',
editor: MapZoomEditor,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for slider

@ryantxu
Copy link
Member Author

ryantxu commented Jul 9, 2021

@torkelo / @dprokop -- any objection to getting this merged and sorting out dynamic options and either standard controls for view+zoom, or complex ones that know current position in additional smaller PRs?

I added a few notes and tasks to: #36585

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea we can merge this and iterate and on the rest. and try to apply that nested builders PR dominik opened in a separate PR that looks promising

@ryantxu ryantxu enabled auto-merge (squash) July 9, 2021 15:28
@ryantxu ryantxu merged commit 9ce6e2a into main Jul 9, 2021
@ryantxu ryantxu deleted the spatial-ol branch July 9, 2021 15:53
@dprokop
Copy link
Member

dprokop commented Jul 9, 2021

@torkelo / @dprokop -- any objection to getting this merged and sorting out dynamic options and either standard controls for view+zoom, or complex ones that know current position in additional smaller PRs?

I added a few notes and tasks to: #36585

Go go go!

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

Successfully merging this pull request may close these issues.