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

Put layer switcher into sidebar-v2 #14

Closed
mstenta opened this issue Jul 12, 2019 · 22 comments
Closed

Put layer switcher into sidebar-v2 #14

mstenta opened this issue Jul 12, 2019 · 22 comments
Assignees
Labels
enhancement New feature or request

Comments

@mstenta
Copy link
Member

mstenta commented Jul 12, 2019

Follow up to #3

See https://github.com/Turbo87/sidebar-v2

@mstenta
Copy link
Member Author

mstenta commented Jul 12, 2019

I tried this, but ran into some issues getting it to work. Worth another try in the future...

@mstenta mstenta added the enhancement New feature or request label Jan 20, 2020
@paul121
Copy link
Member

paul121 commented Sep 9, 2020

Oh hey! Just recently came across the ol-layerswitcher Sidebar demo and was going to open an issue for this exact feature! We're currently using the ol-ext library to add a legend to the map. This works OK, but makes it challenging to add additional UI components to the map. The Sidebar would be great to have in farmOS-map since it would provide a cohesive space for adding additional UI features/behaviors within the Map container. It also seems to work well on mobile!

Before finding this issue I was going to propose including a sidebar property in the options object of createInstance({target, options}). That would allow the Sidebar to only be rendered in contexts that want the additional UI. But perhaps we could default to including the Sidebar in all farmOS-map views to be more consistent?

I haven't tried integrating this with farmOS-map yet, but it seems like it will require some thinking. In the demo I linked to above, the Sidebar is largely defined in separate HTML divs: A high level sidebar-content div, with "panes" defined in child sidebar-pane divs (html source). The high level sidebar-content div is referenced when building the Sidebar control (js source):

var sidebar = new ol.control.Sidebar({ element: 'sidebar', position: 'left' });

Some questions/considerations that come to mind...

  • How to integrate this with the farmOS-map API?
    • Can farmOS-map generate the Sidebar HTML itself?
    • Should an HTML element for the Sidebar be provided when creating an instance of farmOS-map (similar to how the maps target element is provided)? Allowing the Sidebar element to be provided might allow for more flexibility with placing & styling the Sidebar?
  • How will other map behaviors provide additional Panes for the Sidebar?
    • Have behaviors append sidebar-pane elements under the sidebar-content element?
    • Create an internal API such as instance.addSidebarPane({id, name, content}) that generates the sidebar-pane and sidebar-header elements, which might help maintain consistent styling?
  • How would this affect multiple maps? Should each sidebar & pane have unique IDs?
  • Would this affect current farmOS-map behaviors & control elements? Eg: Should edit controls be moved into the sidebar?

Would love to hear your thoughts @mstenta @jgaehring !

@mstenta
Copy link
Member Author

mstenta commented Sep 10, 2020

Great summary of the considerations @paul121 ! When I was first starting farmOS-map I found sidebar-v2 and got excited about making it a default part of the maps, for exactly the reasons you describe. I did some quick testing to see if it would be an easy drop-in, but realized that it needed additional markup and just stopped there (wasn't a priority to get to parity with farmOS for https://www.drupal.org/project/farm/issues/3083352). I'm still interested in exploring/considering it! Thanks for giving this thought - looks like we've got our work cut out for us. :-)

@jgaehring
Copy link
Member

This is cool!

The Sidebar would be great to have in farmOS-map since it would provide a cohesive space for adding additional UI features/behaviors within the Map container.

I guess I'm still trying to think what kinds of UI features / behaviors those would be. Any examples?

Can farmOS-map generate the Sidebar HTML itself?

I kind of like the separation between the map and the markup, and would vote nay on rolling the markup into farmOS-map itself. I'm still not sure how I would use this in FK, but if I were, I'd certainly prefer the freedom to write the markup with Vue and incorporate the component library for consistency of look and behavior.

The markup seems like a fun challenge though! I would probably nitpick and suggest that the sidebar, when closed, does not need to span the entire vertical length of the page, like it does in the example. ;-)

@mstenta
Copy link
Member Author

mstenta commented Sep 10, 2020

I guess I'm still trying to think what kinds of UI features / behaviors those would be. Any examples?

Apart from the layer switcher itself, I also thought it might be a nice space to put some help text / map user guide - directly in context, but hidden in a sidebar tab.

I wonder if it could also provide a space for information that might normally go into a popup too. Eg: imagine clicking on a geometry for an animal's location and seeing more info/options in the sidebar. The popups are a bit more constrained in their size... and they are also limited to a single popup. Perhaps sidebar could accommodate multiple geometry selections. And maybe even include actions like "Move these animals". Thinking big here... ;-)

But if we think about Field Kit specifically: I like how we are able to keep the map relatively simple and focused on the particular use-cases we need in each field module. So making the sidebar optional would make sense to me.

The layer switcher works fine as-is right now in Field Kit - especially considering we include much fewer layers in FK maps - so moving it to a sidebar might not really improve much. It's more helpful in farmOS where there are sometimes lots of layers, and the layer switcher needs to scroll/collapse groups to make it manageable.

@jgaehring
Copy link
Member

I wonder if it could also provide a space for information that might normally go into a popup too. Eg: imagine clicking on a geometry for an animal's location and seeing more info/options in the sidebar. The popups are a bit more constrained in their size... and they are also limited to a single popup. Perhaps sidebar could accommodate multiple geometry selections.

Oh nice! I really like that idea. Kind of how Google maps uses the sidebar when you click on a business. Shows pictures, profile info, etc. We could basically put a mini version of the area's page right in there. Cool!

@paul121
Copy link
Member

paul121 commented Sep 10, 2020

Great ideas! I like including help text, as well as using the sidebar as an extension of the "popup" feature. And area/asset "bios" haha

One specific use case we're interested in is viewing sensor data & log quantities associated with an area. The sidebar would give more space to display a small graph or even tabular data. We're also curious about creating a "tool" that helps highlight areas of interest on the map based on different properties. For example, high/low sensor values, specific logs that need to be completed, etc.. just having space to put simple Form elements like this would be very helpful.

Also with Drone & Satellite imagery the number of layers can add up quick. So a UI to search & filter those based on date & area is another feature that could use that space.

@jgaehring re: Using Vue, thats a good consideration.. reason for making this more flexible maybe. We're also pretty interested in getting custom imagery layers into FK, which might be easier to implement with the Sidebar, but understand that feature may be a little ways out!

@mstenta
Copy link
Member Author

mstenta commented Sep 10, 2020

Oh yea! Perhaps the timeline slider controls would fit better inside a sidebar too: #53

@jgaehring
Copy link
Member

We're also pretty interested in getting custom imagery layers into FK, which might be easier to implement with the Sidebar, but understand that feature may be a little ways out!

Oh nice! More I think of it, the more I'm on board with bringing this into FK. Again, like Google maps, I think it'd work pretty well as a drawer that slides up from the bottom, rather than the side. Which is another reason to keep it separate and flexible. I'd love to explore that more if your interested. I'd really like to split out the the Movements tab to its own module, too. Perhaps that'd be a good space to experiment with the sidebar/drawer as well.

@jgaehring
Copy link
Member

Oh yea! Perhaps the timeline slider controls would fit better inside a sidebar too: #53

Oh man, or mstenta/farm_crop_plan#28!

@mstenta
Copy link
Member Author

mstenta commented Sep 21, 2020

Cross-linking to this comment: #87 (comment)

Perhaps the snapping grid control would be a good feature to consider moving into a sidebar, along with some help text.

@paul121
Copy link
Member

paul121 commented Sep 21, 2020

Some issues getting the sidebar library to work as an ES module with ol5+:

Turbo87/sidebar-v2#143

Turbo87/sidebar-v2#144 (comment)

@symbioquine
Copy link
Collaborator

Just saw this issue (again?).

Anyway, having this sidebar could be especially awesome in that it could provide a place where farmOS-map behaviors could expose settings.

Specifically, I'm leaning towards having a setting which would enable the touch drawing features I've been working on - and I think this sidebar might be the right place to put that...

Is anyone working on this and/or tried integrating that sidebar library recently?

@mstenta
Copy link
Member Author

mstenta commented Apr 26, 2021

Go for it! I'm still very curious to see what this could provide.

Of course, we should also look at that library to see if making it one of our dependencies is wise. Looks like the last commits by the maintainer were over a year ago. Might be "feature complete" though? Always need to be mindful of what complexities we might be pulling into our own project when we adopt dependencies. Worth considering "rolling our own" perhaps too, if our needs are simpler and the maintenance tradeoff makes sense.

@mstenta
Copy link
Member Author

mstenta commented Apr 26, 2021

Also very important to consider all other use-cases for farmOS-map, and how this might affect them. Field Kit might not want this, for example. And other non-farmOS projects are starting to use this library too. The further we get with this the more desire I have to keep the core farmOS-map library as simple as possible, but make it easier to "add on" features via "behaviors" or other mechanisms in individual apps. We've discussed one of the primary challenges to this: the fact that we package OpenLayers in the library itself, so it isn't available to other JS libs unless they are also packaged together. Maybe we need to consider those ideas deeper and come up with a longer-term strategy for simplifying the ongoing maintenance of farmOS-map, while also allowing more experimentation in "contrib" - like we do with farmOS core+modules.

@symbioquine
Copy link
Collaborator

Of course, we should also look at that library to see if making it one of our dependencies is wise. Looks like the last commits by the maintainer were over a year ago. Might be "feature complete" though? Always need to be mindful of what complexities we might be pulling into our own project when we adopt dependencies.

Agreed. I'm pretty cautious of taking a dependency on that library. It doesn't look like it would work out-of-the-box without some sort of repackaging step and it doesn't seem like the author has been very receptive to contributions which would modernize its packaging.

Worth considering "rolling our own" perhaps too, if our needs are simpler and the maintenance tradeoff makes sense.

I actually started playing around with how hard it would be to do this last night. The library is quite simple really. Most of the complexity comes from the how the API works - where the caller provides an existing DOM element declaring the whole sidebar/tab structure.

That API isn't really a good fit for farmOS-map anyway, so a rewrite seems somewhat justifiable.

Also very important to consider all other use-cases for farmOS-map, and how this might affect them. Field Kit might not want this, for example. And other non-farmOS projects are starting to use this library too. The further we get with this the more desire I have to keep the core farmOS-map library as simple as possible, but make it easier to "add on" features via "behaviors" or other mechanisms in individual apps.

It seems pretty clear that a sidebar won't be a good fit for all use-cases and would need to be an optional part of farmOS-map's behavior.

That gets a little complex in the case of something like the layer switcher. We'd need to determine if farmOS-map is responsible for saying; "if both the layer switcher and sidebar are enabled, put the layer switcher in the sidebar instead of as an independent control" or if that's the client's responsibility to configure it one way or the other.

My gut feeling is that the sidebar should expose some sort of API on the farmOS-map instance for registering new tabs/contents and the layer switcher behavior should include an option like placement="(auto|standalone|sidebar)" so the calling page can control how it appears.

We've discussed one of the primary challenges to this: the fact that we package OpenLayers in the library itself, so it isn't available to other JS libs unless they are also packaged together. Maybe we need to consider those ideas deeper and come up with a longer-term strategy for simplifying the ongoing maintenance of farmOS-map, while also allowing more experimentation in "contrib" - like we do with farmOS core+modules.

Yeah, I'm also curious whether we can find a way to solve for the packaging/extension conundrum. I'm hoping to post some ideas on that thread again soon...

@paul121
Copy link
Member

paul121 commented Apr 26, 2021

The library is quite simple really. Most of the complexity comes from the how the API works - where the caller provides an existing DOM element declaring the whole sidebar/tab structure. ... That API isn't really a good fit for farmOS-map anyway, so a rewrite seems somewhat justifiable.

This is what I gathered from the last time I looked at it. +1

That gets a little complex in the case of something like the layer switcher. We'd need to determine if farmOS-map is responsible for saying; "if both the layer switcher and sidebar are enabled, put the layer switcher in the sidebar instead of as an independent control" or if that's the client's responsibility to configure it one way or the other.

Yeah, I was going to say it would be hard to make a "sidebar" fully optional. Maybe these parts could be excluded via tree shaking for apps like FK that might not need it.

@symbioquine
Copy link
Collaborator

Yeah, I was going to say it would be hard to make a "sidebar" fully optional.

Actually, that's looking more and more imminently achievable. Check out my recent comments on #68

@paul121
Copy link
Member

paul121 commented Apr 29, 2021

Yeah, I was going to say it would be hard to make a "sidebar" fully optional.

Actually, that's looking more and more imminently achievable. Check out my recent comments on #68

Ah nice! I hadn't seen the latest comments over there haha thanks for pointing that out.

Even though I mentioned tree shaking I think I was thinking more about the "how to determine if the sidebar should be used" logic in my head. But maybe that's kinda trivial... farmOS-map have some logic like "if the sidebar behavior is available, put the layer switch in the sidebar" else "normal layer switcher" ?

@symbioquine
Copy link
Collaborator

Status update: I've published a mostly rewritten version of sidebar-v2 as https://github.com/symbioquine/ol-side-panel and am planning to work on a farmOS-map behavior PR which integrates it next...

@symbioquine symbioquine self-assigned this Jun 26, 2021
@symbioquine
Copy link
Collaborator

I've pushed some experimental changes to my fork...

Peek 2021-06-27 12-44

@mstenta
Copy link
Member Author

mstenta commented Sep 6, 2021

Closing this as outdated. @symbioquine implemented a new approach here: #124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

4 participants