Skip to content

BGDIINF_SB-2501 : add support for embed URL param#407

Merged
pakb merged 2 commits intodevelopfrom
feat-BGDIINF_SB-2808_embed-UI
Jun 13, 2023
Merged

BGDIINF_SB-2501 : add support for embed URL param#407
pakb merged 2 commits intodevelopfrom
feat-BGDIINF_SB-2808_embed-UI

Conversation

@pakb
Copy link
Contributor

@pakb pakb commented May 25, 2023

when set to true, the UI is simplified to a minimum (zoom in zoom out and a link to open the app in a new tab)

I had to rework a bit the URL parsing tooling for this, as otherwise the embed=false or embed= were staying in the URL. But working on this opened another whole can of worms with app startup and I had to change the logic in some places in the storeSync plugin to fix that (layers and bgLayers were disappearing at startup somehow)

Test link

when set to true, the UI is simplified to a minimum (zoom in zoom out and a link to open the app in a new tab)

I had to rework a bit the URL parsing tooling for this, as otherwise the embed=false or embed= were staying in the URL. But working on this opened another whole can of worms with app startup and I had to change the logic in some places in the storeSync plugin to fix that (layers and bgLayers were disappearing at startup somehow)
@pakb pakb requested a review from ltshb May 25, 2023 09:50
@ltshb
Copy link
Contributor

ltshb commented May 30, 2023

@pakb Would it not be better to also allow embed to be an alias to embed=true ? This would reduce a bit the size of the url.

@ltshb
Copy link
Contributor

ltshb commented May 30, 2023

@pakb switching infobox of a feature to the header don't work, the infobox disappear and the user need to refresh the windows to see it again.

@ltshb
Copy link
Contributor

ltshb commented May 31, 2023

@pakb After some thoughts if we need to touch the URL parameter logics, I think it would be wise to try to simplify it. It seems a bit complex to me especially with the keepValueInUrlWhenEmpty flag which is used in several place in code and not always straightforward. I'm thinking of a more generic way to handle parameters, something as follow:

  1. Add a defaultValue member to the AbstractParamConfig
  2. Change the keepValueInUrlWhenEmpty to keepInUrlWhenDefault
  3. Always add the param to the store, either from query if found or using the defaultValue. In this logic no need to check for keepInUrlWhenDefault
  4. When writing the URL, only add non default param unless keepInUrlWhenDefault is true
  5. Make sure that boolean param without value are treated as true same as boolean param with value 1

This should allow to have the embed flag in url without value to keep url short. It would also allow to have embed=true or embed=1. We could also have a flag which is per default true if needed in future. Maybe we can have a quick call to discuss this idea ?

Insead of choosing between keeping an empty value in the URL or not, we now can define which is the default value of a param, and if it should stay in the URL when the value in the store is the default value.
@pakb pakb force-pushed the feat-BGDIINF_SB-2808_embed-UI branch from 92be64b to a977700 Compare June 13, 2023 07:42
@pakb pakb requested a review from ltshb June 13, 2023 07:58
@pakb
Copy link
Contributor Author

pakb commented Jun 13, 2023

Should be as you have suggested, I've modified the URL param parser so that flags can be set to true just by being present in the URL (no need to =true anymore) and also changed the logic and replaced keepValueInUrlWhenEmpty with a keepInUrlWhenDefault

@ltshb
Copy link
Contributor

ltshb commented Jun 13, 2023

@pakb I guess the old embed (old viewer) translation to the new viewer will be done via redirect ? embedeed.html -> #?embed, am I correct. Has it already be done ? If not is there already a ticket ?

Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

Looks great thanks

@pakb
Copy link
Contributor Author

pakb commented Jun 13, 2023

@pakb I guess the old embed (old viewer) translation to the new viewer will be done via redirect ? embedeed.html -> #?embed, am I correct. Has it already be done ? If not is there already a ticket ?

that's a good point, as it was a different HTML endpoint in mf-geoadmin3 I think we might need to tackle that on the proxy/redirect side of things (what Bernie was working on recently), I'll create a ticket so that we do not forget to deal with it.

@pakb pakb merged commit 69b22d6 into develop Jun 13, 2023
@pakb pakb deleted the feat-BGDIINF_SB-2808_embed-UI branch June 13, 2023 12:21
@pakb pakb mentioned this pull request Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants