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

4632 - Adding DAP script tag into the CKAN footer template #186

Merged
merged 9 commits into from
Mar 12, 2024

Conversation

dlennox24
Copy link
Contributor

@dlennox24 dlennox24 requested a review from FuhuXia March 1, 2024 17:03
@dlennox24 dlennox24 self-assigned this Mar 1, 2024
@btylerburton
Copy link
Contributor

@FuhuXia
Copy link
Member

FuhuXia commented Mar 1, 2024

@FuhuXia can we add remote URL's to webassets? https://github.com/GSA/ckanext-datagovtheme/blob/main/ckanext/datagovtheme/fanstatic_library/webassets.yml

Not sure, but even we can, it does not sound like a good idea to let the server fetch a remote js and bundle it. We can always let user browser to fetch it like we did in the the touchpoint form.

@btylerburton
Copy link
Contributor

Good point. What template is it that handles adding the webassets to the bottom of the page? We could add it there.

@FuhuXia
Copy link
Member

FuhuXia commented Mar 1, 2024

Good point. What template is it that handles adding the webassets to the bottom of the page? We could add it there.

Either that, or we can create a wrap js to $.getScript() the dap js.
So the purpose for the change is that you don't like adding an external js in the <footer> somewhere in the middle the html body?

btylerburton
btylerburton previously approved these changes Mar 1, 2024
Copy link
Contributor

@btylerburton btylerburton left a comment

Choose a reason for hiding this comment

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

🧹

@btylerburton
Copy link
Contributor

Looks like there is a failure to find this repo for UI test though. I can help troubleshoot more when I'm back on my machine.

pull access denied for ckan/ckan-solr-dev, repository does not exist or may require 'docker login': denied: requested access to the resource is denied

@dlennox24 dlennox24 changed the title 4632 - Adding script tag into the CKAN footer template 4632 - Adding DAP script tag into the CKAN footer template Mar 11, 2024
FuhuXia
FuhuXia previously approved these changes Mar 11, 2024
FuhuXia
FuhuXia previously approved these changes Mar 11, 2024
@FuhuXia
Copy link
Member

FuhuXia commented Mar 11, 2024

Can you deploy the two latest changes to catalog-dev?

@tdlowden
Copy link
Member

@FuhuXia do we need to change the branch it's merged into in order to deploy to dev?

@jbrown-xentity
Copy link
Contributor

do we need to change the branch it's merged into in order to deploy to dev?

@dlennox24 @tdlowden:
You'll need to incorporate these changes into catalog.data.gov repo (see here, and will need to pull directly from this repo and branch instead of pypi, there are other examples in the requirements file). Once you have that in a branch/PR, you can push that to the develop branch using git push origin branch_name:develop, and this will use github action based on the develop branch. Let me know if we need to pair on this!

@jbrown-xentity
Copy link
Contributor

Also, we'll need to bump the version of this extension in this PR before it get's merged: https://github.com/GSA/ckanext-datagovtheme/blob/main/setup.py#L13

@tdlowden
Copy link
Member

Since @dlennox24 is not working on this til Friday but we'd like to progress, maybe we can grab some time together @jbrown-xentity. I have a conflict during pair time today but I'll check your cal!

@FuhuXia FuhuXia merged commit b8dc495 into main Mar 12, 2024
4 checks passed
@FuhuXia FuhuXia deleted the 4632_resolve-dap-script-regression branch March 12, 2024 16:43
@tdlowden
Copy link
Member

Can confirm I see the DAP script firing in dev
image

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.

5 participants