Add async hide/show check for custom tabs #4167
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alisman
force-pushed
the
custom_tabs
branch
2 times, most recently
from
1d0c3ee to
20d38ec
Compare
pvannierop
reviewed
Feb 16, 2022
my-index.ejs
Outdated
Comment on lines
17
to
25
|
|
||
| // serverConfig:{ | ||
| // | ||
| // // custom_tabs:[ | ||
| // // { | ||
| // // "title": "Custom Tab", | ||
| // // "location": "RESULTS_PAGE", | ||
| // // "mountCallbackName": "renderCustomTab1", | ||
| // // "hide":false, |
There was a problem hiding this comment.
Should this file be committed?
| @computed get customTabs() { | ||
| return prepareCustomTabConfigurations( | ||
| getServerConfig().custom_tabs, | ||
| 'COMPARISON_PAGE' |
There was a problem hiding this comment.
So, you extended the custom tabs to Comparison Page? Nice would be good to make that clean in docs and release notes.
Comment on lines
+748
to
+766
| // {getServerConfig().custom_tabs && | ||
| // getServerConfig() | ||
| // .custom_tabs.filter( | ||
| // (tab: any) => | ||
| // tab.location === 'PATIENT_PAGE' | ||
| // ) | ||
| // .map((tab: any, i: number) => { | ||
| // return ( | ||
| // <MSKTab | ||
| // key={getPatientViewResourceTabId( | ||
| // 'customTab' + i | ||
| // )} | ||
| // id={getPatientViewResourceTabId( | ||
| // 'customTab' + i | ||
| // )} | ||
| // unmountOnHide={ | ||
| // tab.unmountOnHide === | ||
| // true | ||
| // } |
There was a problem hiding this comment.
Remove commented lines?
Comment on lines
+273
to
+274
| studyViewRegex.test(location.pathname) || | ||
| customTabRegex.test(location.pathname) |
There was a problem hiding this comment.
Just asking, but should there not be a comparisonView test as well?
There was a problem hiding this comment.
i'll make a ticket for it and do it separately
|
|
||
| runTests('ResultsView', resultsUrl, 'RESULTS_PAGE'); | ||
|
|
||
| runTests('PatientView', patientUrl, 'PATIENT_PAGE'); |
There was a problem hiding this comment.
I noticed that Comparison View is also supported not. Should we not include a test for it?
alisman
force-pushed
the
custom_tabs
branch
3 times, most recently
from
3ed77c5 to
1bcaa6b
Compare
pvannierop
approved these changes
Feb 18, 2022
alisman
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a way to configure custom tabs such that an async check can be run to see whether or not the tab should show for a given view of the page. E.g. we should rerun this check when the query changes on the result page or when the patient/sample change on patient page.
The hideAsync property is a function which MUST return a promise. When the promise resolves with a boolean value, we will either show or hide the tab button. Note that if the user is already routed to the tab, we should always show the button even if the hydeAsync function returns false. In that case we should render some N/A placeholder for the tab content.