-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor getSupportedChartTypes() to clarify the rules by chartType #2359
base: master
Are you sure you want to change the base?
Conversation
…nstead of using procedural code
server/src/termdb.server.init.js
Outdated
supportedChartTypes[r.cohort].add('cuminc') | ||
if (numericTypes.has(r.type)) numericTypeCount[r.cohort] += r.samplecount | ||
} | ||
const isSupportedChart = { |
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 a good design. it can be improved by removing profile charts. this should only define general chart types that can be determined by term type availability. profile charts and any other special purpose charts are only defined in their own ds.cohort.specialCharts[]. and will be added to supportedChartTypes
as-is.
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.
So that would mean that any chartType that's listed in specialCharts
, is not meant to be be listed in hiddenCharts
, right? In other words, if it's listed in specialCharts
, just ignore hiddenCharts
for it.
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.
correct. special charts will simply be added and will not be explicitly coded here, this simplifies logic
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.
Maybe we can have a method that does a switch with cases for the charts with more complex logic and a default behaviour, something like:
isSupported(chart)
{
switch(chart):
...
default:
return specialCharts.includes(chart) && !hiddenCharts.includes(chart)
}
Description
... instead of using procedural code. Closes #2229
Tested with TermdbTest only, other datasets will need to be migrated in
sjpp
repo. The chart buttons in localhost should match the ones in prp1 (11 charts).Tested with http://localhost:3000/testrun.html?name=*.unit and
cd client && npm run test:integration
.Checklist
Check each task that has been performed or verified to be not applicable.