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

Update "List of URLs" download option label to "List of HTTP URLs" to… #2670

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ggalibert
Copy link

… make the used protocol clearer and in preparation for a potential future support for a "List of OPeNDAP URLs". Also reflected this change in the retrieved file name.

Note that I didn't change the content of downloadUrlListAction in web-app/js/portal/lang/en.js since @jonescc believes this is used by google analytics and it would then be problematic.

Finally, not sure whether I should also update the "List of URLs" entries to "List of HTTP URLs" in src/test/javascript/portal/cart/DownloadPanelSpec.js.

… make the used protocol clearer and in preparation for a potential future support for a "List of OPeNDAP URLs".
@jonescc
Copy link
Contributor

jonescc commented Jul 17, 2018

The instances of "List of URLs" in src/test/javascript/portal/cart/DownloadPanelSpec.js refer to the google analytics label which hasn't changed, so no need to change that.

The build is failing, but its also failing in master and isn't related to these changes.

@pblain, the code changes look fine, assuming we shouldn't change the label used for List of URL's in google analytics as this is a cosmetic change.

Not sure who needs to sign off on a label change.

@pblain
Copy link
Contributor

pblain commented Jul 19, 2018

@jonescc - Correct - we definitely don't want to change the GA label. Changing it would screw up the stats. Once you choose GA event naming conventions you're stuck with them, which is why we need to get them right in the first place. We haven't always got them right unfortunately.

@ggalibert
Copy link
Author

Then if that's OK can someone merge this PR?

@lbesnard
Copy link

lbesnard commented Aug 4, 2021

@ccmoloney this is old but still relevant. Can someone look at this and merge if it's not breaking the portal?

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