Skip to content

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

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

Merged
merged 4 commits into from
Mar 11, 2024
Merged

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

merged 4 commits into from
Mar 11, 2024

Conversation

ltshb
Copy link
Contributor

@ltshb ltshb commented Mar 7, 2024

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

Test link

Copy link

cypress bot commented Mar 7, 2024

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 👍

Copy link
Contributor

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
Now we use a new view for the embed instead of a query parameter.
Some tests are flaky therefore increase the retries.
@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants