Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Qmaps 844 + QMAPS-845 add points anywhere on the map (+ panel with favorite / share / directions / categories buttons) #289

Merged
merged 27 commits into from
Aug 13, 2019

Conversation

xem
Copy link
Contributor

@xem xem commented Jul 10, 2019

Capture d’écran de 2019-08-12 14-58-05

Copy link
Contributor

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Semi-colons are missing a bit everywhere.

GuillaumeGomez
GuillaumeGomez previously approved these changes Aug 12, 2019
const poiId = placeUrlMatch[1];
poiId = placeUrlMatch[1];
}
if (poiId && !poiId.startsWith('latlon:')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this change is no longer necessary, now that Idunn API can handle latlon: places

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed 6d7bb0e

{{= this.PoiBlocContainer.render(this.poi) }}

{{? this.poi.isFromOSM() }}
{{? isAnywherePoi || (this.poi.isFromOSM && this.poi.isFromOSM()) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

this block should not be visible with anywhere POIs: OSM view and edit URLs are not defined for them

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed a00d7b7

width: 100px;
height: 100px;
width: 90px;
height: 90px;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really have to reduce the image size, that is already quite small ?

Copy link
Contributor

Choose a reason for hiding this comment

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

🤷‍♂️ I don't even see the cases where it's used, so I don't have any opinion. Maybe we can simply revert this change for now to merge, and discuss every UI tweaks in a following PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the IRL explanation, so this is the illustration picture (most often photo) that exists for some POIs. I agree this is important, so I reverted it at 100px for now. 90dc60f
I'll probably change the layout of this component later so it doesn't depend on fixed height, so we don't have a big empty space when this photo doesn't exist.

@jbgriesner jbgriesner merged commit 2880b68 into master Aug 13, 2019
@amatissart amatissart deleted the QMAPS-844 branch September 11, 2019 14:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants