Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing implementation of wagtailcharts #7250

Draft
wants to merge 29 commits into
base: main
Choose a base branch
from
Draft

Conversation

csebianlander
Copy link
Contributor

This is a draft PR designed to test the wagtailcharts package as a potential replacement or supplement to our Simple Charts module.

Right now, the rendering template is not working, because of reasons presumably beyond my understanding or expertise, but you can add, edit, and save the block properly in the page editor (I've only added it to the Browse page type for now just for testing purposes).

@csebianlander
Copy link
Contributor Author

You'll need to run pip install wagtailcharts on your local environment before testing.

@chosak
Copy link
Member

chosak commented Sep 21, 2022

@csebianlander I think I got this working thanks to some helpful JS tips from @anselmbradford. Ans, if you have a minute, can you take a glance at my JS and see if it seems reasonable?

I'm able to get working charts on a BrowsePage (for example http://localhost:8000/admin/pages/12070/edit/) with a configuration like this:

image

This generates a chart like this:

image

You'll need to yarn build to properly build the frontend script for this.

@csebianlander
Copy link
Contributor Author

I've made a lot of notes and have a lot of thoughts about this. The most prevalent is that it's a wonderful improvement from a back-end usability perspective.

We can add our CFPB-specific palette using the ChartBlock(colors=COLORS) argument described in the wagtailcharts docs, but my attempt to implement this led to some weird artifacts, so I'd love it someone who knows better what they're doing could make this change and add this set of colors:

    COLORS = (
        ('#20aa3f', 'Green'),
        ('#0072ce', 'Pacific'),
        ('#5a5d61', 'Gray'),
        ('#257675', 'Teal'),
        ('#254b87', 'Navy'),
        ('#b4267a', 'Purple'),
    )

Some things I wonder if we can integrate simply into this test:

  • Additional/custom fields
  • SimpleCharts-based styling, to implement our fonts and color styles.
  • Style/function overrides, either in the editor or (probably more practically) within the wagtail template? The more we can manipulate the charts.js API in our testing, the more easily we can convey to the wagtailcharts folks what we've found in terms of potential fixes and implementations for field options.

@@ -354,7 +354,7 @@ def page_js(self):
# Returns the JS files required by this page and its StreamField blocks.
@property
def media_js(self):
return sorted(set(self.page_js + self.streamfield_media("js")))
return list(dict.fromkeys(self.page_js + self.streamfield_media("js")))
Copy link
Member

Choose a reason for hiding this comment

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

This change allows us to preserve the order of JS script loads for a block, for example given a block like

class MyBlockWithJS(blocks.StructBlock):
    class Media:
        js = ["b.js", "a.js"]

Our template will currently sort these and generate tags like

<script src="/static/js/routes/on-demand/a.js"></script>
<script src="/static/js/routes/on-demand/b.js"></script>

This causes problems if something in a.js relies on something in b.js being loaded first, as is the case with wagtailcharts.

I suppose in theory this change might break some alphabetized dependency we didn't know we needed, but I think it's better to be explicit with ordering.

Copy link
Member

Choose a reason for hiding this comment

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

The only place I thought this might cause an issue is with our video player loading youtube's iframe API but the loading order doesn't matter because all the youtube script does is load a second script that explicitly has an async attribute.

So this change looks a-okay. 👍

@@ -336,6 +336,8 @@
{% for js in page.media_js %}
{% if 'https://' in js %}
s.push('{{ js }}');
{% elif js.endswith('?staticroot') %}
Copy link
Member

Choose a reason for hiding this comment

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

This is just a placeholder approach and there's definitely a better way to do this. After #7303 we assume either that a path is its own URL if it contains https:// or lives in js/routes/on-demand/ if not. The wagtailcharts block type needs to load its own JS file that is being served out of the static root, so neither of these work. Per @anselmbradford's comment on #7303 (comment), maybe this is sufficient motivation to adopt paths as objects?

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.

4 participants