feat: read-only geolocation (GDE-86) #16561

barredterra · 2022-04-10T14:18:54Z

Continues work from #16513

Depends on #21055

Render the map into the control's display_area
Hide draw controls if the map is read-only
Remove the useless refresh button
Separate get_leaflet_controls and point_to_layer into separate methods that can be more easily overwritten with a custom script

Before

Read-Write

Read only

After

Read-Write

Read only

Todo

map not displayed after hard-refresh (CMD + SHIFT + R)
- Map is displayed after cur_frm.reload_doc(). Maybe this doesn't get triggered on hard refresh?
- No problems in version 13
read-only map doesn't load on empty data (fix: Exclude Geolocation from "hide empty read-only field" #21088)
this.map.invalidateSize() probably needs to get called at a different time. (feat: read-only geolocation (GDE-86) #16561 (comment))

Scenarios to test

Does the map render nicely in all of the following scenarios?

Map in collapsible and normal section
Refresh, hard-refresh, or page through records
Editable and read-only map
With and without data/drawings

no-docs

Internal reference: LAN-756

- render map into display_area - hide draw controls if read-only - remove useless refresh_button

stale · 2022-04-17T16:21:24Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

shariquerik · 2022-04-22T08:37:16Z

@barredterra Are you working on the Todo part you mentioned?

barredterra · 2022-04-22T08:46:12Z

@shariquerik I'm currently out of ideas / don't know where to start searching 😅. Do you have any ideas?

shariquerik · 2022-04-27T11:03:57Z

@shariquerik I'm currently out of ideas / don't know where to start searching 😅. Do you have any ideas?

I will have to look into it.
Btw hide draw controls if read-only is not working.

barredterra · 2022-04-27T16:42:57Z

Btw hide draw controls if read-only is not working.

Are you sure you're loading the most recent code? Tested this several times and using it in production v13 already.

stale · 2022-05-04T18:52:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

frappe/public/js/frappe/form/controls/geolocation.js

codecov · 2022-05-11T11:14:20Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@a049b7f). Click here to learn what that means.
The diff coverage is 0.00%.

❗ Current head c9eb03f differs from pull request most recent head 64d1e64. Consider uploading reports for the commit 64d1e64 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             develop   #16561   +/-   ##
==========================================
  Coverage           ?   58.78%           
==========================================
  Files              ?      769           
  Lines              ?    68989           
  Branches           ?     6005           
==========================================
  Hits               ?    40554           
  Misses             ?    24949           
  Partials           ?     3486

Flag	Coverage Δ
ui-tests	`49.74% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

hrwX · 2022-05-11T14:48:10Z

@barredterra Am I missing something here ? When I make this field read_only, the field disappears

Screencast.from.05-11-2022.03_43_24.PM.mp4

hrwX · 2022-05-11T15:23:57Z

@barredterra the refresh_input in base_control.js is not triggered => set_input in data.js is not triggered => set_disp_area in data.js is not triggered and hence we cannot see the map on CMD + SHIFT + R press

stale · 2022-05-24T11:07:27Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

…olocation render - When gelocation control is built, it awaits loading of libraries - Withi the control everything is await but a layer above where multiple controls are sequentially built this breaks - Form render goes ahead without waiting for the gelocation control - The `onload_post_render` in `Location` cannot find the map wrapper as the control is still being built - Therefore, on a hard load, the control does not show up and appears on soft reload.

blaggacao · 2023-05-28T20:55:52Z

@barredterra Looks like this would need a rebase after #21055 is merged, now.

As I needed the patch, I've done a rebase here.

On this occasion: Whats the rationale of using set_disp_area(value) over format_for_input(value)?

barredterra · 2023-05-30T17:33:36Z

Whats the rationale of using set_disp_area(value) over format_for_input(value)? – @blaggacao

Long time since i wrote this... 🤔 From the naming, I guess format_for_input was not getting called for read-only fields.

barredterra · 2023-05-30T17:35:25Z

@marination could you review this PR?

marination · 2023-05-31T13:21:05Z

@barredterra Replicated the this.map.invalidateSize() issue. It occurs on geolocation controls that are inside collapsible sections. This is the only pending issue in this PR. Code looks fine and functions fine.

What happens is:

invalidateSize() essentially tries to find the width and height of the map wrapper so that it can make the map occupy the entire wrapper space.
If the control is in a collapsed section, the wrapper is hidden and therefore invalidateSize() gets a width and height of 0 and thus it essentially does nothing.
When you expand the section, some grey tiles are visible due to this invalidateSize() being ineffective

So the "right" time to invoke invalidateSize() is when the map control is not hidden anymore i.e. on the collapse() event of the section

frappe/public/js/frappe/form/controls/geolocation.js

This way, the logic can stay in the control itself. Each control can decide what it needs to do on section collapse/expand.

For better customizability

barredterra · 2023-06-03T12:50:01Z

@marination thanks a lot for your investigation and the suggested fix. Everything seems to be working fine now. 👍🏼

marination · 2023-06-05T05:58:46Z

@barredterra tested all scenarios. LGTM. @surajshetty3416 can you give it a final look and merge ?

surajshetty3416 · 2023-06-05T12:26:53Z

LGTM. Merging.

feat: read-only geolocation (GDE-86)

5171d6e

- render map into display_area - hide draw controls if read-only - remove useless refresh_button

stale bot added the inactive label Apr 17, 2022

barredterra removed the inactive label Apr 17, 2022

Merge branch 'develop' into map-read-only

2558585

barredterra marked this pull request as ready for review April 17, 2022 16:38

barredterra requested review from a team and surajshetty3416 and removed request for a team April 17, 2022 16:38

surajshetty3416 assigned shariquerik Apr 18, 2022

barredterra requested a review from hrwX April 25, 2022 18:43

stale bot added the inactive label May 4, 2022

barredterra removed the inactive label May 4, 2022

barredterra added 2 commits May 4, 2022 22:33

Merge branch 'develop' into map-read-only

95116b8

Merge branch 'develop' into map-read-only

51dc7f5

surajshetty3416 assigned barredterra May 11, 2022

Merge branch 'develop' into map-read-only

69893a8

hrwX reviewed May 11, 2022

View reviewed changes

frappe/public/js/frappe/form/controls/geolocation.js Outdated Show resolved Hide resolved

stale bot added the inactive label May 24, 2022

barredterra removed the inactive label May 24, 2022

barredterra marked this pull request as draft May 24, 2022 11:21

marination and others added 3 commits May 22, 2023 14:53

refactor: const instead of var, indentation

d6edc15

fix: handle read only property

50b15be

barredterra unassigned barredterra and shariquerik May 22, 2023

barredterra requested a review from marination May 22, 2023 13:10

marination mentioned this pull request May 23, 2023

fix: Exclude Geolocation from "hide empty read-only field" #21088

Merged

barredterra added the backport version-14-hotfix backport to version 14 label May 23, 2023

frappe unlocked this conversation May 24, 2023

Merge branch 'develop' into map-read-only

5a89e8c

barredterra assigned marination May 30, 2023

marination reviewed May 31, 2023

View reviewed changes

frappe/public/js/frappe/form/controls/geolocation.js Outdated Show resolved Hide resolved

barredterra added 3 commits June 3, 2023 14:16

refactor: call control's on_section_collapse()

ba22512

This way, the logic can stay in the control itself. Each control can decide what it needs to do on section collapse/expand.

fix: fit and recenter map when section is expanded

79aaf07

refactor: move pointToLayer into a separate method

f9251f0

For better customizability

barredterra requested a review from marination June 3, 2023 12:38

Merge branch 'develop' into map-read-only

64d1e64

marination approved these changes Jun 5, 2023

View reviewed changes

surajshetty3416 merged commit e5524ba into frappe:develop Jun 5, 2023

mergify bot mentioned this pull request Jun 5, 2023

feat: read-only geolocation (GDE-86) (backport #16561) #21249

Merged

barredterra deleted the map-read-only branch June 5, 2023 12:52

ankush mentioned this pull request Jun 5, 2023

map read only #21144

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: read-only geolocation (GDE-86) #16561

feat: read-only geolocation (GDE-86) #16561

barredterra commented Apr 10, 2022 •

edited

Loading

stale bot commented Apr 17, 2022

shariquerik commented Apr 22, 2022

barredterra commented Apr 22, 2022

shariquerik commented Apr 27, 2022 •

edited

Loading

barredterra commented Apr 27, 2022

stale bot commented May 4, 2022

codecov bot commented May 11, 2022 •

edited

Loading

hrwX commented May 11, 2022

hrwX commented May 11, 2022

stale bot commented May 24, 2022

blaggacao commented May 28, 2023 •

edited

Loading

barredterra commented May 30, 2023 •

edited

Loading

barredterra commented May 30, 2023

marination commented May 31, 2023

barredterra commented Jun 3, 2023

marination commented Jun 5, 2023

surajshetty3416 commented Jun 5, 2023

feat: read-only geolocation (GDE-86) #16561

feat: read-only geolocation (GDE-86) #16561

Conversation

barredterra commented Apr 10, 2022 • edited Loading

Before

Read-Write

Read only

After

Read-Write

Read only

Todo

Scenarios to test

stale bot commented Apr 17, 2022

shariquerik commented Apr 22, 2022

barredterra commented Apr 22, 2022

shariquerik commented Apr 27, 2022 • edited Loading

barredterra commented Apr 27, 2022

stale bot commented May 4, 2022

codecov bot commented May 11, 2022 • edited Loading

Codecov Report

hrwX commented May 11, 2022

hrwX commented May 11, 2022

stale bot commented May 24, 2022

blaggacao commented May 28, 2023 • edited Loading

barredterra commented May 30, 2023 • edited Loading

barredterra commented May 30, 2023

marination commented May 31, 2023

barredterra commented Jun 3, 2023

marination commented Jun 5, 2023

surajshetty3416 commented Jun 5, 2023

barredterra commented Apr 10, 2022 •

edited

Loading

shariquerik commented Apr 27, 2022 •

edited

Loading

codecov bot commented May 11, 2022 •

edited

Loading

blaggacao commented May 28, 2023 •

edited

Loading

barredterra commented May 30, 2023 •

edited

Loading