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

Feature/1338 anchor bug fix #1339

Merged
merged 5 commits into from
May 31, 2023
Merged

Feature/1338 anchor bug fix #1339

merged 5 commits into from
May 31, 2023

Conversation

OlofSvahnVbg
Copy link
Collaborator

This PR refers to this issue: Oversiktsplan - Url link missing from Anchor View (Dela) #1338

closes #1338

@OlofSvahnVbg OlofSvahnVbg added this to the 3.x milestone May 26, 2023
@OlofSvahnVbg OlofSvahnVbg self-assigned this May 26, 2023
@jacobwod
Copy link
Member

Can you please check if the behaviour is still correct when enableAppStateInHash is enabled? To enable it, just add the query string param enableAppStateInHash, e.g. https://karta.halmstad.se/?enableAppStateInHash.

Do the live hash update and the Anchor plugin (both its initial value and live updating of the input in AnchorView) still work?

@OlofSvahnVbg
Copy link
Collaborator Author

So I tried https://oversiktsplan-utv.varberg.se/?/enableAppStateInHash and the anchor gets set the same way as it does without the query, and the live update seems to work aswell (if you mean registering changes like moving the map, zooming etc.)

@jacobwod
Copy link
Member

That sound good, thanks for the test. We'll take a proper look next week!

componentDidMount() {
// We check if anchor is a string, to avoid error in render() below.
// This because the getAnchor() sometimes returns a Promise instead of a string.
typeof anchor === "string" &&
Copy link
Member

Choose a reason for hiding this comment

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

anchor is not defined here and won't be of type string. When i test your implementation, the view results in an empty field:
image

(It renders an empty field if you have the anchor plugin enabled at start).

I think this issue could be resolved with:

async componentDidMount() {
    const a = await this.props.model.getAnchor();
    this.setState({ anchor: a });
    // Subscribe to changes to anchor URL caused by other components. This ensure
    // that we have a live update of the anchor whether user does anything in the map.
    this.props.globalObserver.subscribe(
      "core.mapUpdated",
      ({ url, source }) => {
        this.setState({
          anchor: this.appendCleanModeIfActive(url),
        });
      }
    );
  }

Maybe you could test that and see if it works for you as well? The strange thing is that the original implementation also works for me...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. I tried having the dela-window opened at start aswell, and the link does appear. But I'll try your fix :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So your codes seems to be working fine. Although, if I scroll while the page is starting the link does not appear.

Copy link
Member

Choose a reason for hiding this comment

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

You mean it doesn't get updated?

The componentDidMount part runs only on initial component load. The following updates of anchor string are taken care of elsewhere, please ensure to use the same methodology (await this.props.model.getAnchor()) before calling setState().

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... Seems like getAnchor never resolves in some cases... It's not pretty but this should do the trick:

async componentDidMount() {
    // Subscribe to changes to anchor URL caused by other components. This ensure
    // that we have a live update of the anchor whether user does anything in the map.
    this.props.globalObserver.subscribe(
      "core.mapUpdated",
      ({ url, source }) => {
        this.setState({
          anchor: this.appendCleanModeIfActive(url),
        });
      }
    );
    const a = await this.props.model.getAnchor();
    this.setState({ anchor: a });
  }

(We have to make sure the subscription is made. If getAnchor never resolves, the subscription is never made, and the anchor-url is never updated).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works on my end :)

@Hallbergs Hallbergs merged commit 462eb62 into develop May 31, 2023
@Hallbergs Hallbergs deleted the feature/1338-AnchorBugFix branch May 31, 2023 05:16
This pull request was closed.
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.

3 participants