Skip to content

PB-314: Moved the &embed to #/embed#694

Merged
ltshb merged 4 commits intodevelopfrom
feat-PB-314-embed
Mar 11, 2024
Merged

PB-314: Moved the &embed to #/embed#694
ltshb merged 4 commits intodevelopfrom
feat-PB-314-embed

Conversation

@ltshb
Copy link
Contributor

@ltshb ltshb commented Mar 7, 2024 •
edited by github-actions bot
Loading

Now we use a new view for the embed instead of a query parameter.

Test link

@cypress
Copy link

cypress bot commented Mar 7, 2024 •
edited
Loading

Passing run #1003 ↗︎

0 169 22 0 Flakiness 0

Details:

PB-314: Minor code changes based on code review
Project: web-mapviewer Commit: 066fe02515
Status: Passed Duration: 04:38 💡
Started: Mar 8, 2024 4:09 PM Ended: Mar 8, 2024 4:14 PM

Review all test suite changes for PR #694 ↗︎

@ltshb ltshb force-pushed the feat-PB-314-embed branch 2 times, most recently from 613fa66 to 83f0ad6 Compare March 7, 2024 10:56
@ltshb ltshb requested a review from pakb March 7, 2024 11:39
@ltshb ltshb marked this pull request as ready for review March 7, 2024 11:39
@ltshb ltshb force-pushed the feat-PB-314-embed branch 2 times, most recently from cc46c04 to b25be6b Compare March 8, 2024 13:29
@pakb
Copy link
Contributor

pakb commented Mar 8, 2024

At first glance, by playing with the test link :
great improvement! thanks

I saw that you filtered out some part of the UI that were forgotten (when they were added...) I think we should also remove the BG selector when in embedded (check an iframe example from mf-geoadmin3 here : https://codepen.io/geoadmin/pen/yOBzqM )

I'm totally OK not having the 3D button 👍

pakb
pakb reviewed Mar 8, 2024
View reviewed changes
src/router/storeSync/storeSync.routerPlugin.js
Copy link
Contributor

@pakb pakb Mar 8, 2024

Choose a reason for hiding this comment

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

As a global comment on this file, I'm not sure we shouldn't make our view names constant in view/view.js and never use a hard-coded string. It might be more resilient to future renaming of the views

@ltshb
Copy link
Contributor Author

ltshb commented Mar 8, 2024

At first glance, by playing with the test link : great improvement! thanks

I saw that you filtered out some part of the UI that were forgotten (when they were added...) I think we should also remove the BG selector when in embedded (check an iframe example from mf-geoadmin3 here : https://codepen.io/geoadmin/pen/yOBzqM )

I'm totally OK not having the 3D button 👍

The footer clean up and button will be done in separate PR/tickets

ltshb added 4 commits March 8, 2024 17:05
@ltshb
PB-314: Moved the &embed to #/embed
58498b9 
Now we use a new view for the embed instead of a query parameter.
@ltshb
PB-314: Added e2e tests for embed
4813eea
@ltshb
Increased the number of retries of cypress
3def376 
Some tests are flaky therefore increase the retries.
@ltshb
PB-314: Minor code changes based on code review
066fe02
@ltshb ltshb force-pushed the feat-PB-314-embed branch from b25be6b to 066fe02 Compare March 8, 2024 16:06
@ltshb ltshb requested a review from pakb March 8, 2024 16:06
@ltshb ltshb merged commit e5cf47d into develop Mar 11, 2024
@ltshb ltshb deleted the feat-PB-314-embed branch March 11, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pakb pakb pakb approved these changes

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ltshb @pakb