Skip to content

Conversation

iamareebjamal
Copy link
Member

@iamareebjamal iamareebjamal commented Jan 4, 2020

Fixes #3761

@iamareebjamal
Copy link
Member Author

@prateekj117 @kushthedude Please review and test as I have to release this after merging

{{/g-map-address-marker}}
{{/g-map}}
{{#if (eq mapConfig.display 'embed') }}
<iframe class="g-map" src="https://maps.google.com/maps?q={{event.locationName}}&t=&z=15&ie=UTF8&iwloc=&output=embed" frameborder="0" scrolling="no"></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

Is it temporary @iamareebjamal ,as we are not using any custom calls in the app? If yes, we should just add a comment untill mapbox is implemented, as it will break the coding conventions getting followed for handlebars in the repository.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is not temporary. If there is neither mapbox nor google API, this will be rendered. As both services are paid and this is free.

as we are not using any custom calls in the app?

What do you mean by custom calls? Google Maps API is a custom library and we are using it anyway

as it will break the coding conventions getting followed for handlebars in the repository

What coding conventions for handlebars, can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing,I didnt saw the iframe tag thought it is js tweak.
It will be better to add some text between iframe tag, if iframe is not supported in any device?

Copy link
Member Author

@iamareebjamal iamareebjamal Jan 4, 2020

Choose a reason for hiding this comment

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

If iframe is not supported in that device, then the device probably does not even have a working browser. iframe is an HTML4 element which was launched in December 18, 1997 and probably older than you

Copy link
Member

Choose a reason for hiding this comment

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

😅 Its lgtm

@niranjan94
Copy link
Member

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- config/environment.js  3
         

See the complete overview on Codacy

@iamareebjamal iamareebjamal merged commit cf53569 into fossasia:development Jan 4, 2020
@iamareebjamal iamareebjamal deleted the map branch January 4, 2020 16:19
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.

Fallback to embedded iframe map if no API key

4 participants