-
Notifications
You must be signed in to change notification settings - Fork 279
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
RFC87: add button to load GroupComparison table data in external tool #4938
Conversation
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
68b7318
to
4c4bdc4
Compare
- added margin to seperate tool section from section header. - made clear for "Windows" and visible
- icon - display, copying standard button - style - WIP: not config driven and always appears
- ExternalToolConfig.ts to localize in one place - IAppConfig.external_tools: pull in type definition - serverConfigDefaults: pull in default
- changed to be config-driven - support array - removed local image and pull from URL (avoids relative path)
- renamed files to match folder (camelCase) - handleDownloadWrapper WIP - shrunk icon to be closer to the other
- change to class - rename folder to match convention - moved render() out of CopyDownloadButtons - changed CopyDownloadControls to pass along the downloadData instead of handleDownload to give tool control - restored local icon so not dependent on external
- iconImageSrc local fixed - have to use require() - put urlFormat parameters in a new object
- cleanup - use clipboard
- dataLength validation - getSingleStudyName
- mock clipboard - mock navigation - changed function references to functions for jest - fixed urlParameters indexing complaints
- isAvailable check - required_platform - added fontdetect - localStorage caching
Removed unnecessary imports
- changed to launch url with open() so uses new tab - add alert() if opening URL fails (probably won't happen) - added Test Tool to config - fixed showing multiple <ExternalTool>
- moved getServerConfig() dependency to a new file to fix dependency issue with tests - removing unnecessary imports
…ttern EXPL: the "$" complicates parsing when running in JS.
- fix one leftover old term - fixed CustomButtonConfig to catch all interface properties
src/config/IAppConfig.ts
Outdated
@@ -186,4 +186,5 @@ export interface IServerConfig { | |||
vaf_log_scale_default: boolean; // this has a default | |||
skin_study_view_show_sv_table: boolean; // this has a default | |||
enable_study_tags: boolean; | |||
custom_buttons_json: string; |
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.
This will be defined by the backend application.properties.
src/config/config.ts
Outdated
@@ -340,7 +340,7 @@ export function initializeServerConfiguration(rawConfiguration: any) { | |||
); | |||
} catch (err) { | |||
// ignore | |||
console.log('Error parsing localStorage.frontendConfig'); | |||
console.log('Error parsing localStorage.frontendConfig:' + localStorage.frontendConfig); |
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.
Useful update to debugging if "localStorage.frontendConfig" testing is not working.
width: 550px; | ||
padding-right: 40px; | ||
} | ||
} |
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.
this class is used by the "3rd party" section - it is the same as toolArray but wider, which looks better here.
.customButtonImage { | ||
width: 18px; | ||
height: 18px; | ||
} |
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.
This is to make sure that whatever size the image is, the "button" is basically the same size as the Copy+Download font awesome buttons.
return false; | ||
}; | ||
} | ||
} |
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.
This utility is a workaround to basically check a boolean "installed" flag set on the client system. It appears there isn't a way to check if a particular URL protocol is supported, or a particular software is installed, so this was the workaround that I found. The software installs a special font, then the frontend code can check if that font is installed by indirectly checking if a fallback font is used (by looking at sizes).
This code is modified from the original to be more efficient.
ALTERNATIVES
A) We could remove the FontDetector code, then the CustomButton would appear depending on UserAgent only (e.g. all Windows browsers). That could work, since I believe we can detect if launching the URL protocol failed then direct the user to another page.
B) If there is another way to detect if a particular URL protocol is supported or a particular software is installed, then we could try that. However, my research into the issue seemed to conclude there wasn't.
); | ||
}); | ||
} | ||
|
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.
We display the components here following the Download button. I couldn't get the "" working in the HTML code so I moved the showDownload test to this function. I believe it's better that way anyways.
return this.props.downloadData().then(data => data.text); | ||
} | ||
} | ||
|
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.
The click handler for CustomButton needs the actual data, so downloadData() is passed to the component. There are two ways that buttons get data though, synchronous (SimpleCopyDownloadControls) and asynchronous (CopyDownloadControls). So we set pass downloadData as a Promise.
Comments related to this are tagged as "TECH_DOWNLOADDATA".
OPTIONAL: technically, CustomButton could take a Promise<ICopyDownloadData>, but then we would have to move that interface out of CopyDownloadControls.tsx and I wanted to minimize the changes.
@@ -20,4 +20,7 @@ export interface ICopyDownloadInputsProps { | |||
downloadLabel?: string; | |||
handleDownload?: () => void; | |||
handleCopy?: () => void; | |||
// expose downloadData() to allow button to handle the data on it's own. | |||
// TECH_DOWNLOADDATA: CopyDownloadButtons.downloadData needs to be async so it can work with either async context (IAsyncCopyDownloadControlsProps) or synchronous context (SimpleCopyDownloadControls) | |||
downloadData?: Promise<string>; |
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.
See comment in CopyDownloadControls.tsx about why downloadData is a Promise
return Promise.resolve(this.props.downloadData()); | ||
} | ||
} | ||
|
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.
See comment in CopyDownloadControls.tsx about why downloadData is a Promise. Here we have a wrapper to convert the synchronous case to a Promise.
renamed property to download_custom_buttons_json |
- renamed ICopyDownloadControls and ICustomButton property downloadDataAsync to be clear is async - fixed downloadDataAsPromise() to not run downloadData() until resolved
…cBioPortal#4938) * Visualize Your Data: add 3rd party tools section, link, and image. * Allow for configuration of custom buttons in data tables that send data to third-party tools on client system
…cBioPortal#4938) * Visualize Your Data: add 3rd party tools section, link, and image. * Allow for configuration of custom buttons in data tables that send data to third-party tools on client system
Summary
This PR addresses all requirements for RFC87, which involves conditionally adding a button to the GroupComparison data tables (only visible to relevant users), then launching the external tool with the data.
This PR implements a config-driven solution for defining "CustomButtons", so anyone can update their instance application.properties to launch their own custom URL protocol/software with the table data. This PR alone will not impact the appearance of the website without the "custom_buttons_json" config being defined.
Key changes:
Testing Notes
To test this PR with a CustomButton, if the backend application.properties don't define custom_button_json, then you can use either of the following methods.
Browser Console
Paste the following into the browser console, then reload the page.
Bookmarklet
Create a bookmarklet that will run the netlify for this PR and set the config.
Tests
With the above configuration, the button will only appear on a windows system with the software (and custom font) installed.
Changes Summary
Configuration
CopyDownloadButtons Update
New Component
Global State Dependency
Considerations
Some things to consider that we could update.
Checks
Notify reviewers
@alisman, @jjgao