Skip to content

Feature/map#172

Open
LBHMVillalobos wants to merge 20 commits intodevelopfrom
feature/map
Open

Feature/map#172
LBHMVillalobos wants to merge 20 commits intodevelopfrom
feature/map

Conversation

@LBHMVillalobos
Copy link
Copy Markdown

  • lbh-map component amended:
    1. If uprn is provided, the lat/long will be taken from the addresses API. If not provided, it uses the provided lat/long
    2. New basemap and attribution
  • lbh-block-component amended to use the new lbh-map.

Please contact me to get the OS_BASEMAP_TOKEN and the ADDRESS_API_PROXY_URL for testing.

height: 400px;
order: 4;

@include govuk-media-query($from: desktop) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this specific sizing, to support lbh-map within the lbh-contact-block? If so I would rather target this behaviour from lbh-contact-block styling. Making it the full width behaviour by default.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

full width is the default now and the size of the contact-block is managed in the contact block scss

Comment thread lbh/components/lbh-map/_map.scss Outdated
}
}

&__map-container_full_width {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Following BEM this should be a modifier, so &__map-container--full-width. Thats if it's still necessary with the above comment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed - it is no longer necessary with the above change

Comment thread lbh/components/lbh-map/map.js Outdated
this.moduleId = this.$module.getAttribute("id");
this.accessToken = this.$module.getAttribute("data-access-token");
this.moduleId = this.$module.getAttribute("lbh-map");
//this.accessToken = process.env.OS_BASEMAP_TOKEN; //Process env not working yet. Please copy/paste the OS_BASEMAP_TOKEN in the meantime
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the idea that the env is available at build time? Once this is compiled, process is not available in the browser.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've removed the .env comments

Comment thread lbh/components/lbh-map/map.js Outdated
if (this.uprn){
//Process env not working yet. Please copy/paste the ADDRESSES_API_PROXY_URL in the meantime
//fetch(process.env.ADDRESSES_API_PROXY_URL+"?format=detailed&uprn="+this.uprn, {
fetch("ENTER_THE_ADDRESS_API_PROXY_URL_HERE?format=detailed&uprn="+this.uprn, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason we don't hardcode this url? Feels like the query parameters are tied to the url (unless this is an open standard)?.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I've hardcoded the url

fetch("ENTER_THE_ADDRESS_API_PROXY_URL_HERE?format=detailed&uprn="+this.uprn, {
method: "get"
})
.then(response => response.json())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens if there is a 400-500 response status? Can we handle that exception?

Comment thread lbh/components/lbh-map/_map.scss Outdated
}
}

&__map-container {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The repetition of map here seems unnecessary as the scope is lbh-map. I think this was just carried over from lbh-contact-block.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, it was carried over from the contact-block. It has been updated.

Comment thread lbh/components/lbh-map/_map.scss Outdated
overflow: hidden;
}

&__title {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see this being used in the example html? Is this necessary or can we update the html?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed. It was not necessary

@LBHMVillalobos LBHMVillalobos marked this pull request as ready for review August 5, 2021 16:36
@cloudratha
Copy link
Copy Markdown
Contributor

Hi @LBHMVillalobos, I totally forgot about this. Seeing as MMH has no immediate need for this it might be worth reaching out to other members that have implemented the previous iteration to review? I wouldn't know who that would be unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants