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

Embedded geostory URL issue with scroll position #9047

Open
ElenaGallo opened this issue Mar 22, 2023 · 6 comments · May be fixed by #10369
Open

Embedded geostory URL issue with scroll position #9047

ElenaGallo opened this issue Mar 22, 2023 · 6 comments · May be fixed by #10369

Comments

@ElenaGallo
Copy link
Contributor

ElenaGallo commented Mar 22, 2023

Description

The embedded geostory is not visible if the Advanced Options of the direct share link are enable.

How to reproduce

  • Open a geostory and follow the steps in the video below:
embedded.geostory.mov

Expected Result

The URL of the embedded geostory does not contain the scroll position

Other useful information

@tdipisa
Copy link
Member

tdipisa commented Mar 22, 2023

Thank you @ElenaGallo. @dsuren1 can you please investigate/estimate this?

@tdipisa tdipisa added this to the 2023.01.01 milestone Mar 22, 2023
@dsuren1
Copy link
Contributor

dsuren1 commented Mar 23, 2023

Investigation
This is not a regression (also happens in v2022.02.00), as there was no implementation that specifically handles the removal of the query params when url generated for geostory-embedded. By default all the query params applicable for shared url is present on embedded url.

The scroll information is removed from the shared url, only when Include scroll position is set to false. This validation is unmodified

 const shouldRemoveSectionId = !settings.showSectionId && advancedSettings && advancedSettings.sectionId;
 let shareUrl = getSharedGeostoryUrl(removeQueryFromUrl(this.props.shareUrl), shouldRemoveSectionId);

We can add a specific config in embedOptions or advancedSettings perhaps to remove query params from shared url to form a embedded-url, there by handling this scenario in an isolated manner
localConfig:

{
  geostory: [
    {
      "name": "Share",
      "cfg": {
        "embedOptions": {
          "removeQueryFromShared": true
        }
      }
    }
  ]
}

@tdipisa Kindly let me know your thoughts.

@tdipisa
Copy link
Member

tdipisa commented Mar 23, 2023

@dsuren1 thank you.

as there was no implementation that specifically handles the removal of the query params when url generated for geostory-embedded. By default all the query params applicable for shared url is present on embedded url.

I'm sorry but it is not clear to me. What do you mean precisely? The expected behavior in this case would not be to 'remove' the section path in case of embedded URL. The expected behavior would be to have it working by scrolling to the desired section also for embedded gestories.

@dsuren1
Copy link
Contributor

dsuren1 commented Mar 23, 2023

As per the issue description, the expected result is

The URL of the embedded geostory does not contain the scroll position and the home button

@tdipisa
Copy link
Member

tdipisa commented Mar 23, 2023

As per the issue description, the expected result is

The URL of the embedded geostory does not contain the scroll position and the home button

@dsuren1
you are right, I'm sorry. I did't see it and probably I forgot that there is some missing implementation that is preventing the embedded URL to work with /section/id endpoint. I don't remember why precisely, we should check, but it is not the case here now since for this issue the identified behavior is effectively a regression from what we previously released: I suppose that's the reason why we decided remove the /section/id endpoint from the embedded URL.
I can confirm that because I've tested with binaries of previous versions until 2021.01.03 where effectively the /section/id was not included in the embedded URL. The showHome query parmeter yes instead, but it is not preventing the embedded URL to work as expected: that parameter is simply ignored if present.

If you could spend 1h more to identify the reason of the regression providing also an estimate for the fix that would be good.

@dsuren1
Copy link
Contributor

dsuren1 commented Mar 23, 2023

@tdipisa

section-id

From the gif, you can see that section id is not added when user is on the first section but added when scrolled to second. This is because the section id and column id is parsed from the url of the page here instead of the fetching the value from state->geostory-> currentPage
image

There are three issues,

  • The section id is not added to shared url when user is on title/first page of geostory i.e when url doesn;t have section/column info
  • The section id is not updated in the shared url when user scrolls with Include scroll position option enabled
  • There is no implementation in place, to remove section id from geostory-embedded url when Include scroll position is enabled that adds section id to sharedUrl

This behavior exist on previous versions as well. This scenario can be easy to miss and probably overlooked and not happening just recently.

@tdipisa tdipisa removed this from the 2023.01.01 milestone Mar 23, 2023
@tdipisa tdipisa added this to the 2024.01.01 milestone May 20, 2024
@tdipisa tdipisa modified the milestones: 2024.01.01, 2024.01.02 May 28, 2024
@Igi-ID Igi-ID linked a pull request May 29, 2024 that will close this issue
4 tasks
@Igi-ID Igi-ID linked a pull request May 29, 2024 that will close this issue
4 tasks
@tdipisa tdipisa assigned mahmoudadel54 and unassigned Igi-ID Jun 10, 2024
@tdipisa tdipisa removed this from the 2024.01.02 milestone Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants