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.

Sign up for GitHub

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

Jump to bottom

Support MapSwipe in Permalink #5166

Merged
merged 5 commits into from
Oct 8, 2019
Merged

Support MapSwipe in Permalink #5166

merged 5 commits into from
Oct 8, 2019

Conversation

RBcote
Copy link
Contributor

@RBcote RBcote commented Sep 25, 2019 •
edited
Loading

This patch introduce 'mapswipe' as a parameter for the permalink service.

@RBcote RBcote requested a review from adube September 25, 2019 16:09
@adube adube changed the title 1029-Permalink wip [WIP] - Support MapSwipe in Permalink Sep 25, 2019
adube
adube suggested changes Sep 30, 2019
View reviewed changes
contribs/gmf/src/permalink/Permalink.js Outdated Show resolved Hide resolved
@adube
Copy link
Contributor

adube commented Sep 30, 2019

@RBcote Please, provide a description to the PR and remove [WIP] from the title.

@RBcote RBcote changed the title [WIP] - Support MapSwipe in Permalink Support MapSwipe in Permalink Sep 30, 2019
adube
adube suggested changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@adube adube left a comment

Choose a reason for hiding this comment

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

Tested: upon loading it is not working.

In the url, I ended up with a map_swipe=undefined value, which toggled on the map swipe component on nothing.

Please, fix.

@adube adube requested a review from fredj September 30, 2019 17:27
adube
adube approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@adube adube left a comment

Choose a reason for hiding this comment

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

Code is clean, and it is also working properly. Please, wait for @fredj approval before merging.

@fredj Please review.

@adube
Copy link
Contributor

adube commented Oct 1, 2019

@fredj ping

@sbrunner sbrunner added this to the 2.5 milestone Oct 4, 2019
@adube
Copy link
Contributor

adube commented Oct 7, 2019

@fredj Would you please check this PR as well? Thanks.

@adube adube force-pushed the T9226-1-Mapswipe-permalink branch from 32a1a94 to 99d1185 Compare October 7, 2019 13:58
@adube
Copy link
Contributor

adube commented Oct 7, 2019

Required correction has been fixed. Rebased onto master.

Will merge as 1 commit after Travis is happy.

@fredj
Copy link
Member

fredj commented Oct 7, 2019

@adube all builds are currently failing on travis

@adube adube force-pushed the T9226-1-Mapswipe-permalink branch from 99d1185 to 7801c1c Compare October 7, 2019 14:11
@adube
Copy link
Contributor

adube commented Oct 7, 2019

@adube all builds are currently failing on travis

@fredj Okay, then I suppose I won't be able to merge. If you know how, please do.

@fredj
Copy link
Member

fredj commented Oct 7, 2019 •
edited
Loading

it should be ok now

@adube adube merged commit 4ee9180 into master Oct 8, 2019
@adube adube deleted the T9226-1-Mapswipe-permalink branch October 8, 2019 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adube adube adube approved these changes

@fredj fredj fredj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.
4 participants
@RBcote @adube @fredj @sbrunner