-
Notifications
You must be signed in to change notification settings - Fork 34
Implements export view to excel button #1202
Conversation
Downloads all selected rows as excel spreadsheet
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.
Hi @pablosichert ,
thanks for the PR. Did a quick review and requested some changes.
wdyt?
src/components/header/SubHeader.js
Outdated
@@ -218,6 +224,9 @@ class Subheader extends Component { | |||
} while (currentNode && currentNode.children && (currentNode.type !== 'window')); | |||
} | |||
|
|||
// TODO: refine gridView conditional | |||
const gridView = parseInt(windowType) === 143; |
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.
atm i would suggest to not enforce it at all. Just let it be enabled for any view.
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.
Another option would be to only enable the button if selected.length > 0
. Or do you want the button the be there when nothing is selected (better discoverabilty of that feature)?
src/components/header/SubHeader.js
Outdated
{gridView && query && ( | ||
<a | ||
className="subheader-item js-subheader-item" | ||
href={`/rest/api/documentView/${windowType}/${query.viewId}/export/excel?selectedIds=${selected.join(',')}`} |
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.
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.
I took the URL schema from your comment - or do you refer to explicitly prepending config.API_URL
? Maybe I'm overlooking something here.
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.
Oh, got it - I need to prepend config.API_URL
and strip /rest/api
from the hardcoded string.
Thanks for looking over the changes. Updated accordingly to your comments. |
thanks! |
#1197