WIP: V1 Samples #13
WIP: V1 Samples #13
Conversation
|
Should probably scrub all the ES6 syntax for compatibility |
Samples/DataSources/datasources.js
Outdated
| // To get dataSource info, first get the dashboard. | ||
| const dashboard = tableau.extensions.dashboardContent.dashboard; | ||
|
|
||
| // Then loop through each worksheet and get its datasources, save promise for later. |
There was a problem hiding this comment.
[nit] looks like you are using "dataSources" in other places, should probably be consistent.
Samples/DataSources/datasources.js
Outdated
| }); | ||
| }, function (err) { | ||
| // Something went wrong in initialization. | ||
| console.log('Error while Initializing: ' + err.toString()); |
There was a problem hiding this comment.
we might want to consider using err.message() here. to string returns the error prefix as well ("Error: ") which is a bit redundant with what you have.
Samples/DataSources/datasources.js
Outdated
| // Refreshes the a given dataSource. | ||
| function refreshDataSource (dataSource) { | ||
| dataSource.refreshAsync().then(function () { | ||
| console.log(dataSource.name + ': Refreshed Successfully'); |
There was a problem hiding this comment.
I haven't ran the sample, so I don't know what it looks like. Are there any UI indications showing the refresh succeeded? If not, creating a popup/alert might be better for demo purposes.
|
just some minor comments. everything else looks good to me. |
|
@jz-huang Thanks for the comments, all good and will address each one. Would you mind looking at the recently pushed filters samples? You are the filter expert on our team :) That one is a reading filterInfo and clearing all filters sample. I thought about doing applying filters as well, but decided we should leave that for another sample as the first one was getting pretty long already. |
|
@lbrendanl definitely. But I don't see the filtering updates. is it in this pr? I still see todo in filtering.js |
…into V1Samples
* removing old images and updating Flex * Fetching is complete * Fix style issues * parity with original demo, but uglier * Address code review feedback * moving to wrapping everything into an anonymous function
|
@jz-huang sorry it should be there now.
Still TODO on here that I already know of:
|
| }); | ||
|
|
||
| function fetchFilters () { | ||
| unregisterHandlerFunctions.forEach(function (unregisterHandlerFunction) { |
There was a problem hiding this comment.
[nit] it's not obvious at first glance what this does. maybe add comment clarifying.
Everything else looks good to me. Thanks for adding these!
graysonarts
left a comment
graysonarts
left a comment
There was a problem hiding this comment.
Mostly just questions on a few things here and there.
Samples/DataSources/datasources.js
Outdated
| infoSpan.className = 'glyphicon glyphicon-info-sign'; | ||
|
|
||
| let showModalFunction = function () { showModal(dataSource); }; | ||
| infoSpan.addEventListener('click', showModalFunction); |
There was a problem hiding this comment.
I think splitting the function out of the addEventListener makes this more confusing, especially since all you are doing is calling a single function.
infoSpan.addEventListener('click', function () { showModal(dataSource); });I think that reads better (at least to my eye...)
There was a problem hiding this comment.
Totally agree now that you mention it, will fix, thanks!
| }); | ||
|
|
||
| function fetchFilters () { | ||
| unregisterHandlerFunctions.forEach(function (unregisterHandlerFunction) { |
There was a problem hiding this comment.
Yeah I think it would be good to have an explanation of what you are unregistering and why.
| unregisterHandlerFunctions.push(unregisterHandlerFunction); | ||
| }); | ||
|
|
||
| Promise.all(filterFetchPromises).then(function (fetchResults) { |
There was a problem hiding this comment.
I think I need to update my sample to use this pattern.
Samples/Filtering/filtering.js
Outdated
| function buildFiltersTable (filters) { | ||
| // Clear the table first. | ||
| $('#filtersTable > tbody tr').remove(); | ||
| const filtersTable = document.getElementById('filtersTable').getElementsByTagName('tbody')[0]; |
There was a problem hiding this comment.
Why do you fall back to DOM access rather than jquery here?
$('#filtersTable > tbody:first') // This should workThere was a problem hiding this comment.
No good reason :) will fix.
| switch (filter.filterType) { | ||
| case 'categorical': | ||
| filter.appliedValues.forEach(function (value) { | ||
| filterValues += value.formattedValue + ', '; |
There was a problem hiding this comment.
// Something to this effect: Not sure if that's more readable or not, but it avoids a bunch of string copying
filterValues = filter.appliedValues.map(function (v) { return v.formattedValue) }).join(', ');
Samples/Filtering/filtering.js
Outdated
| dashboard.worksheets.forEach(function (worksheet) { | ||
| worksheet.getFiltersAsync().then(function (filtersForWorksheet) { | ||
| filtersForWorksheet.forEach(function (filter) { | ||
| worksheet.clearFilterAsync(filter.fieldName); |
There was a problem hiding this comment.
Do you need to wait for these to finish?
* Initial checkin * Initial react checkin * Progressing along * Paste in old readme * React progress * Eventing * Complete functionality * Switch to modal * Add loading screens * readme updates * Part 0 with links * Part 1 readme * Fix links * More tutorials * Part 3 readme * Full res gif * Part 4 * Part 5 * Part 6 and a complete manfiest * Clean up initial readme * Better screenshot for part 3 * Completed screenshot * Delete unused test stuff * Cleanup * Add note about the react version
First sample. Please be as nitpicky as possible since this is sample code.
This sample is fairly simple, and just allows you to set some setting values. Should it do more? We could add delete but to me that felt like an extraneous amount of extra UI code for little API instruction.
Are there any JS best practices I am missing that we should try and promote?