-
Notifications
You must be signed in to change notification settings - Fork 110
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
base: main
Are you sure you want to change the base?
Conversation
You'll need to run |
b111e2f
to
12e0c23
Compare
@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: This generates a chart like this: You'll need to |
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
Some things I wonder if we can integrate simply into this test:
|
a5dd2c9
to
6818728
Compare
@@ -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"))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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') %} |
There was a problem hiding this comment.
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?
Run `yarn build` to rebuild the frontend.
8c23e5f
to
db9cfbc
Compare
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).