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

Add Export Link widget #1997

Conversation

doekenorg
Copy link
Contributor

@doekenorg doekenorg commented Feb 26, 2024

This PR addresses #1891.

It adds a CSV (or TSV) download link widget with a label and option to wrap inside a <p> tag.

Scherm­afbeelding 2024-02-26 om 16 19 01 Scherm­afbeelding 2024-02-26 om 16 19 15 Scherm­afbeelding 2024-02-26 om 16 19 23 ![disabled](https://github.com/GravityKit/GravityView/assets/529515/cb62a001-d07a-4f37-8b0c-9d4bdc04a209)

@zackkatz
Copy link
Member

zackkatz commented Feb 26, 2024

@doekenorg Thanks, this is a good start!

A few things I noticed upon review:

  • Add a setting: "Include all entries" or "Include visible entries". If all entries, it would strip search args. Otherwise, it would keep the search args, but add &csv=1 or &tsv=1 to them.
  • Make sure the link works when Prevent Direct Access is checked
  • When "Allow export" is disabled, show a message in the widget that exporting isn't enabled for the View and provide a JS-connected button to enables it.

A couple of questions:

  • How will this work with DataTables layout?
  • How do you suggest implementing the GravityExport functionality as described in the ticket "Detect if there are GravityExport feeds. For each feed: Enable/disable, Allow customizing link text"
  • Can you see if using add_query_arg()/remove_query_arg() would be a more compatible with the items above than get_permalink()?
  • There's a View setting "Show all in file"—how should we handle this in the widget UI?

@zackkatz
Copy link
Member

This is related to #1560 as well, since we want to integrate an icon picker.

@doekenorg
Copy link
Contributor Author

@zackkatz Ehlers and I noted that the GravityExport support doesn't really make sense here. I created a different issue on that repo to make a separate widget. I feel like it doesn't belong in the gravityview repo.

I'll look into the rest.

@doekenorg
Copy link
Contributor Author

@doekenorg Thanks, this is a good start!

A few things I noticed upon review:

  • Add a setting: "Include all entries" or "Include visible entries". If all entries, it would strip search args. Otherwise, it would keep the search args, but add &csv=1 or &tsv=1 to them.
  • Make sure the link works when Prevent Direct Access is checked
  • When "Allow export" is disabled, show a message in the widget that exporting isn't enabled for the View and provide a JS-connected button to enables it.

A couple of questions:

  • How will this work with DataTables layout?
  • How do you suggest implementing the GravityExport functionality as described in the ticket "Detect if there are GravityExport feeds. For each feed: Enable/disable, Allow customizing link text"
  • Can you see if using add_query_arg()/remove_query_arg() would be a more compatible with the items above than get_permalink()?
  • There's a View setting "Show all in file"—how should we handle this in the widget UI?

@zackkatz I think I'm a little lost here.

This issue is to create a link to the regular view csv / tsv, because people forget to add the correct link. So I'm not sure where these settings are coming from. There are no search parameters. If Prevent Direct Access is checked or if you are on an embeded view, you cannot use that link. There is also no way around this using the REST API for example, because you are not allowed to view the entries if the direct access is denied.

As for datatables, that looks like a whole different ballgame. I don't think this issue and that feature relate to each other.

I can fix the javascript you mentioned, and make sure it also looks for prevent direct access, but other than that; I'm not sure this is the correct issue.

@doekenorg doekenorg self-assigned this Feb 28, 2024
@zackkatz
Copy link
Member

zackkatz commented Feb 29, 2024

@doekenorg Nice! Good start. Cool to see the nonce working.

  • It's not currently passing along the search query to the REST URL, so it doesn't only download searched entries

On the View URL https://dev.test/view/training-plan/?filter_99=.co.uk&mode=any (searching field id 99), the CSV link is currently https://dev.test/wp-json/gravityview/v1/views/45/entries.csv?_nonce=63953260d4. It should be passing along the search parameters (https://dev.test/wp-json/gravityview/v1/views/45/entries.csv?_nonce=63953260d4&filter_99=.co.uk&mode=any).

  • All entries are being downloaded, even when "Show all in output" is not checked. This may be because of the REST endpoint instead of the /csv/ View endpoint. What do you think we should do here?

  • It's rendering the <p> tags for me. I built the CSS and JS and cleared the browser cache. Note: I renamed the widget, which is why it's named something different.

With Export disabled:

Export enabled:

What do you think I'm missing?

@doekenorg doekenorg force-pushed the feature/1891-implement-a-csv-widget-to-be-easily-used-in-the-view branch 3 times, most recently from 646ce64 to 30b08e7 Compare March 4, 2024 14:00
@doekenorg
Copy link
Contributor Author

@zackkatz I've fixed the issues at hand (and removed a bunch of warnings from unit tests). As for All entries are being downloaded. I kinda did that on purpose. So I can remove that, and make it follow the logic that it needs Show all in file enabled as well. I'm just not sure if that makes sense for this feature. Personally, I would assume it downloads all results. Not just the ones currently in view.

So we can either make that explicit with a notice on the widget. Or we can make it follow the same logic as /csv. I'm fine with either.

@doekenorg doekenorg force-pushed the feature/1891-implement-a-csv-widget-to-be-easily-used-in-the-view branch from 993c431 to 8ab3cf1 Compare March 4, 2024 18:45
@doekenorg doekenorg force-pushed the feature/1891-implement-a-csv-widget-to-be-easily-used-in-the-view branch from 8ab3cf1 to 7519a97 Compare March 7, 2024 13:32
doekenorg and others added 10 commits March 7, 2024 14:40
We need to formalize this, but I think we should use our own methods, when we have them available, so that we can implement optimizations.
- Open in same window
- Define `download` attribute to provide browser hinting
- Add `type` attribute for MIME type hinting

In addition, I:

- Switched from sprintf() to strtr() for readability
- Updated the sanitization functions to match content type

https://accessabilly.com/proposal-for-a-more-accessible-download-link/
https://www.nngroup.com/articles/new-browser-windows-and-tabs/#pdfs-and-other-nonhtml-documents
$value can be an array. Since we don't use it, removing the strict type hinting here doesn't matter.
I wanted to update the link URL when searching via DataTables, and it wasn't possible because the link didn't have any default CSS class. To match existing widget structure, I'm adding a `<div>` wrapper.
This allows easy modification in DataTables for when searches are performed.
@zackkatz
Copy link
Member

This should be released in concert with a DataTables change: https://github.com/GravityKit/DataTables/commit/c95170578e57920c87ec22f200e6a66a357174b1

@zackkatz
Copy link
Member

@doekenorg I'm not clear on what "An entry link was added to the CSV output." means in the changelog. Can you elaborate?

@zackkatz zackkatz requested review from mrcasual and zackkatz March 13, 2024 04:08
@doekenorg
Copy link
Contributor Author

@doekenorg I'm not clear on what "An entry link was added to the CSV output." means in the changelog. Can you elaborate?

@zackkatz oh sorry. When downloading a CSV with the REST API, there was an <a> tag around the value of the field that linked to the entry.

@crbdev
Copy link

crbdev commented Mar 15, 2024

Works well for me 👍

@zackkatz zackkatz changed the title Add CSV download link widget Add Export Link widget Mar 18, 2024
@mrcasual mrcasual merged commit 9e134c8 into develop Mar 18, 2024
1 check passed
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