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

QMAPS-759 display POI image on top right if available #168

Merged
merged 13 commits into from
May 7, 2019
Merged

Conversation

xem
Copy link
Contributor

@xem xem commented Apr 8, 2019

No description provided.

Copy link

@Julien-laville Julien-laville left a comment

Choose a reason for hiding this comment

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

missing integration test.
maybe images[0] should be guarded

src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/panel/poi_bloc/images_panel.js Outdated Show resolved Hide resolved
src/views/poi_bloc/images.dot Outdated Show resolved Hide resolved
@jbgriesner
Copy link
Contributor

Thanks for these reviews @Julien-laville !

@GuillaumeGomez
Copy link
Contributor

I have a JS failure when clicking on this element:

Screenshot from 2019-04-11 10-50-21

The JS error is the following:

{…}
​
exception: 0
​
file: "idunn_poi"
​
message: "unknown error getting idunn poi reaching https://maps.dev.qwant.ninja/maps/detail/v1/places/osm:node:4862223479 with options {\"lang\":\"en\"}"
​
method: "poiApiLoad"
​
<prototype>: Object { … }
error.js:41:4
    sendOnce error.js:41
    _callee$ idunn_poi.js:55
    Babel 8

@GuillaumeGomez
Copy link
Contributor

Is there any POI with an image? Even the Eiffel tower doesn't have one...

@GuillaumeGomez GuillaumeGomez force-pushed the QWANT-759 branch 3 times, most recently from 9770c4e to aa4916c Compare April 24, 2019 14:31
Copy link
Contributor

@amatissart amatissart left a comment

Choose a reason for hiding this comment

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

A very quick review.
More css changes will probably be necessary to match the mockup.

src/views/poi_partial/header.dot Outdated Show resolved Hide resolved
@@ -101,6 +99,8 @@ pois:
panelName: internet_access
- apiName: contact
panelName: contact
- apiName: images
panelName: images
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we remove this unused block for now ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't be unused for long so I'd be in favor to keep it for now.

src/views/app_panel.dot Show resolved Hide resolved
src/panel/poi_panel.js Outdated Show resolved Hide resolved
{{?}}

{{? !this.wiki.description && !this.wiki.url }}
<a class="poi_panel__info__wiki__link" rel="noopener noreferrer" target="_blank" href="https://www.wikidata.org/wiki/{{= this.htmlEncode(this.tag) }}">{{= _('Read more on Wikidata') }}</a>
<a class="poi_panel__info__wiki__link" rel="noopener noreferrer" target="_blank" href="https://www.wikidata.org/wiki/{{= this.htmlEncode(this.tag) }}">
<i class="icon-chevrons-right"></i><span>{{= _('WIKIDATA') }}</span></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this second "wikidata" link is here for historical reasons and is never used. It should be removed.

@amatissart amatissart merged commit 14308f4 into master May 7, 2019
@amatissart amatissart deleted the QWANT-759 branch May 20, 2019 11:19
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