Add/chart to popup #73

4lm · 2022-10-12T11:25:44Z

This PR adds dynamic titles, descriptions, charts, and sources to popups.

…ia layers.py in backend

4lm · 2022-10-12T19:11:22Z

I would like to invite you to work with me in this branch (add/chart-to-popup). This branch is all about popups and charts in popups. I created a bar chart popup that fetches data via an AJAX GET request from a fake API endpoint (always same popup data in response). I need your CSS magic to make the popup styling look nice! Can you help me? Please :)

The popup is based on a template named results.html. I prepared the template for you by adding BEM CSS classes, but added zero styling yet. You can touch any class or add new ones, but your are forbidden to change the id attributes that start with "js-" prefixes (as usual).

You can also play with all chart and popup options in popup.js.

Please notice: you have to wait until the app loads and then go to "Ergebnisse" and click on a blackish municipality area for the popup to appear.
Please also notice: It's also OK for me if you extract the necessary code and work in the "RLI Design" repo, if that is easier for you!

Screenie of the unstyled popup with bar chart:

bmlancien · 2022-10-13T09:25:58Z

I would like to invite you to work with me in this branch (add/chart-to-popup). This branch is all about popups and charts in popups. I created a bar chart popup that fetches data via an AJAX GET request from a fake API endpoint (always same popup data in response). I need your CSS magic to make the popup styling look nice! Can you help me? Please :)

Sure, I'd be pleased to work on this :)

4lm · 2022-10-13T17:51:34Z

@bmlancien, a small thing popped into my mind. I integrated the title for the chart, not in the chart, but via an h2-tag above the chart. Therefore there is quite a lot of white space at the top of the chart. I think the title should be integrated into the chart via the JS option object in popup.js.

Can you make the appropriate changes? Drop me a line if you need help with this!

Please also look at this screenie, then you should exactly know what I mean:

… made in #69

bmlancien · 2022-10-17T14:12:09Z

@4lm Thanks for all the explanations. I preferred working on a mockups before starting implementing it. Here is the result:

As mentioned to @nesnoj today, the text would go below the chart because we want to keep the focus on the chart. And the text should also be optional, because in this case (population growth) it is a bit redundant, so I would not even add anything. Important is to have something accessible, like stated in #66, it could for example be the aria-label from echarts that can read charts to screen readers.

Now that I have a mockup, I'm trying to work on the code side, but I am not able to add or change CSS styles to the popup in results.html Basically, trying to do something with the #js-results_popupor .popup divs is not successful. I even cannot see these divs in the Chrome inspector, only mapbox/maplibre classes. I am able to style the content, though, like .popup__title.

Any idea why it is not working?

4lm · 2022-10-18T10:13:12Z

@4lm Thanks for all the explanations. I preferred working on a mockups before starting implementing it.

I love it :)

I'm trying to work on the code side, but I am not able to add or change CSS styles to the popup in results.html

I will have a look and report back!

4lm · 2022-10-18T11:19:46Z

I will have a look and report back!

Works. HTML is there. The results.html is just wrapped into Maplibre-specific popup HTML. The class popup is replaced by maplibregl-popup-content. But the rest of the classes are there. Look at this gist. There I added for you all the HTML that is getting rendered.

I also switched the CSS import order, so you can cleanly overwrite classes from Maplibre (see commit 216ea1f).

Please note: Maplibre is a fork of Mapbox; therefore, the team of Maplibre left all CSS identifiers named mapboxgl-* in there and just added their ones, maplibregl-* Edit: that's why I would target both identifiers so that we can switch easily between both libraries (even though we don't plan to use Mapbox in the future - just in case, I would say, because it makes nearly no extra work).

Please also note: my wire up SCSS example I created for you in digiplan/static/scss/components/_popup.scss

I wish you a happy styling session :)

bmlancien · 2022-10-18T11:51:22Z

Ok, thanks for the explanations @4lm! This is what I thought so, the .popup class is not there anymore, because it is replaced, but I can then target the replacement classes to style the popup.

Please note: Maplibre is a fork of Mapbox; therefore, the team of Maplibre left all CSS identifiers named mapboxgl-* in there and just added their ones, maplibregl-* Edit: that's why I would target both identifiers so that we can switch easily between both libraries (even though we don't plan to use Mapbox in the future - just in case, I would say, because it makes nearly no extra work).

Alright :)

…es made in #77

4lm · 2022-10-18T14:33:11Z

@bmlancien, please not. I made changes to the test API JSON and dependent JS implementation in this PR branch to reflect changes made in #77. Please look those changes made in 391ac4c. Should be self-explanatory, If not, drop me a line, I'm happy to help!

bmlancien · 2022-10-19T12:04:20Z

@4lm I updated the popup with the new design. Please check that I haven't destroyed anything :)

There are also two other things:

The tooltip is doesn't appear on hover. I'm not sure why. I've tried a few things but couldn't make it to work.
I know this can give a headache, but in the best of the worlds, the key values would have a comma or a dot as a thousands separator (depending on the language), or at least a space. Is this something that will be worked on in the future?

4lm · 2022-10-20T10:33:32Z

@4lm I updated the popup with the new design. Please check that I haven't destroyed anything :)

Very nice. Looks good to me. Thank you.

There are also two other things:

OK :)

The tooltip is doesn't appear on hover. I'm not sure why. I've tried a few things but couldn't make it to work.

This is by design. Changing this would also require other changes to make it acceptable to the user. We also had this behavior (click on a municipality to show a popup) in the prior Stemp ABW app. I would suggest keeping it like it is for now, and if we have later time and still want to change this, look at it. OK?

I know this can give a headache, but in the best of the worlds, the key values would have a comma or a dot as a thousands separator (depending on the language), or at least a space. Is this something that will be worked on in the future?

No problem. I will integrate this before I open this PR for review.

4lm · 2022-10-20T11:14:51Z

@henhuy, this PR is ready for review now. Please merge on approval.

henhuy

Thanks for implementing charts in popups. Looks very nice indeed.
Only some little change requests..

henhuy · 2022-10-20T12:12:05Z

digiplan/static/js/popup.js

+            const {chart: {data: {series}}} = response;
+            const xAxisData = createListByName("key", series);
+            const yAxisData = createListByName("value", series);
+            const option = {


I would suggest loading a chart option style file with named options for different charts from outside.
And afterwards fill remaining fields with data from visualization request.
What do you think?

Yes, of course, but not in this PR - I would suggest. This version lacks a proper API and only has one fixed chart type. The code in this PR is a first step, a MacGyver Implementation, held together by a thread, a piece of chewing gum, and some Gaffa tape. I would suggest implementing your suggestion in another PR when we have more data to play with and, therefore, different chart types. As said before, I want to make smaller PRs, to move faster. What do you think? Is this OK for you? I do not insist on my points if they are not absolutely important to me, and this one is NOT absolutely important to me!

henhuy · 2022-10-20T12:13:20Z

digiplan/static/js/popup.js

+              </span>
+            `;
+          }
+          if (field === "description") {


If clauses for title, municipality and description almost look the same - maybe refactor to DRY?

Yes, I also suggest postponing this to a different PR. Because as you can see in the many TODO comments, this code implementation will change a lot in the future. No stone will be left untouched. But for now, I would leave it like this because it makes IMO little sense to change something that will totally be changed in the future. If I keep the if-clauses, work with switch-cases or some function calls is TBD and would be determined in the future. Is that OK for you?

henhuy · 2022-10-20T12:15:01Z

digiplan/templates/map.html

+
+  {{ block.super }}
+
+  {% compress css %}


why did you add a second compress block?

To implicitly make clear to others that there is the possibility to also add css after the super block. I don't know, I felt like it, maybe a YAGNI case?

I will remove the second compress block.

bmlancien · 2022-10-20T14:02:41Z

@4lm

This is by design. Changing this would also require other changes to make it acceptable to the user. We also had this behavior (click on a municipality to show a popup) in the prior Stemp ABW app. I would suggest keeping it like it is for now, and if we have later time and still want to change this, look at it. OK?

No problem for me. Let's see later, maybe with user testing, how it integrated well in the app

No problem. I will integrate this before I open this PR for review.

Cool, thanks! :)

4lm · 2022-10-20T15:47:02Z

@henhuy, from my point of view, this PR is ready for re-review. As written, I would postpone some requested changes to a future PR.

4lm added 9 commits September 26, 2022 21:05

Add basic placeholder chart

212421b

Merge branch 'dev' into add/chart-to-popup

50b0d5f

Merge branch 'dev' into add/chart-to-popup

fc02664

Merge branch 'dev' into add/chart-to-popup

98bec1b

Merge branch 'dev' into add/chart-to-popup

a09766e

Merge branch 'add-metadata-schema-and-tests' into add/chart-to-popup

8ec3351

Add layer id in debug mode in console log on map click

22fef6b

Fetch popup chart data dynamically from static API test endpoint

8fdec99

Construct coordinates from event

d09e23f

4lm mentioned this pull request Oct 12, 2022

Add population forecast schema, example, and tests #69

Merged

4lm added 2 commits October 12, 2022 20:40

Add all popup segments (fields) to popup and make them configurable v…

64b583b

…ia layers.py in backend

Add "js-" prefix to all ids used in popup templates and calling code

7a6a4f1

Merge #69 and update API endpoint placeholder JSON to reflect updates…

19bc74e

… made in #69

4lm added 2 commits October 18, 2022 12:31

Merge branch 'dev' into add/chart-to-popup

957fbc8

Init working with SCSS in popups

216ea1f

4lm mentioned this pull request Oct 18, 2022

Extend popup schema with key values component #77

Merged

Update test API JSON and dependent JS implementation to reflect chang…

391ac4c

…es made in #77

bmlancien added 5 commits October 18, 2022 16:21

start popup styling

8c40b03

merge dev and solve conflicts

29acb93

style popup chart

3d4ce8f

style popup sources

864758d

update popup text to roboto family

e697291

bmlancien added 7 commits October 18, 2022 20:10

finish styling popup for desktop

bda8bf0

merge from dev

bc13ff6

Merge branch 'dev' into add/chart-to-popup

918472d

dynamically populate chart title

bd53070

add rounded corners to region value

bffbdf4

solve conflict in changelog

18e3fe1

add user select none to popup header

de5e478

Add locale numeric rendering to key values components in popups

1f0cb0e

4lm marked this pull request as ready for review October 20, 2022 11:14

4lm requested a review from henhuy October 20, 2022 11:15

henhuy reviewed Oct 20, 2022

View reviewed changes

4lm added 2 commits October 20, 2022 17:38

Merge branch 'dev' into add/chart-to-popup

db58b70

YAGNI, remove unneeded CSS compress block in map HTML template

42ed4ac

4lm requested a review from henhuy October 20, 2022 15:47

henhuy merged commit 330b512 into dev Oct 21, 2022

henhuy deleted the add/chart-to-popup branch October 21, 2022 11:20

nesnoj mentioned this pull request Feb 16, 2023

Add dataset: population #138

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/chart to popup #73

Add/chart to popup #73

4lm commented Oct 12, 2022 •

edited

Loading

4lm commented Oct 12, 2022

bmlancien commented Oct 13, 2022

4lm commented Oct 13, 2022 •

edited

Loading

bmlancien commented Oct 17, 2022

4lm commented Oct 18, 2022

4lm commented Oct 18, 2022 •

edited

Loading

bmlancien commented Oct 18, 2022

4lm commented Oct 18, 2022

bmlancien commented Oct 19, 2022

4lm commented Oct 20, 2022 •

edited

Loading

4lm commented Oct 20, 2022

henhuy left a comment

henhuy Oct 20, 2022

4lm Oct 20, 2022

henhuy Oct 20, 2022

4lm Oct 20, 2022

henhuy Oct 20, 2022

4lm Oct 20, 2022 •

edited

Loading

4lm Oct 20, 2022

bmlancien commented Oct 20, 2022

4lm commented Oct 20, 2022

Add/chart to popup #73

Add/chart to popup #73

Conversation

4lm commented Oct 12, 2022 • edited Loading

4lm commented Oct 12, 2022

bmlancien commented Oct 13, 2022

4lm commented Oct 13, 2022 • edited Loading

bmlancien commented Oct 17, 2022

4lm commented Oct 18, 2022

4lm commented Oct 18, 2022 • edited Loading

bmlancien commented Oct 18, 2022

4lm commented Oct 18, 2022

bmlancien commented Oct 19, 2022

4lm commented Oct 20, 2022 • edited Loading

4lm commented Oct 20, 2022

henhuy left a comment

Choose a reason for hiding this comment

henhuy Oct 20, 2022

Choose a reason for hiding this comment

4lm Oct 20, 2022

Choose a reason for hiding this comment

henhuy Oct 20, 2022

Choose a reason for hiding this comment

4lm Oct 20, 2022

Choose a reason for hiding this comment

henhuy Oct 20, 2022

Choose a reason for hiding this comment

4lm Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

4lm Oct 20, 2022

Choose a reason for hiding this comment

bmlancien commented Oct 20, 2022

4lm commented Oct 20, 2022

4lm commented Oct 12, 2022 •

edited

Loading

4lm commented Oct 13, 2022 •

edited

Loading

4lm commented Oct 18, 2022 •

edited

Loading

4lm commented Oct 20, 2022 •

edited

Loading

4lm Oct 20, 2022 •

edited

Loading