fix(ui): displaying variable list default values #17097
Conversation
ghost
requested review from
|
@asalem1 is this ready for review or should i wait for you to fix the e2e tests? |
|
@hoorayimhelping not quite yet unfortunately. I managed to update and add some tests, but I'm currently trying to add some individual tests for the utility functions I created and bumping into some uncaught existing issues that I'm looking to smooth over. I'll DM you when the PR is ready to go :D |
| const values = getTimeMachineValues(state, ownProps.variableID) | ||
| const {variableID} = ownProps | ||
| let values: VariableValues | ||
| if (variableID.includes('timeRange') || variableID.includes('windowPeriod')) { |
There was a problem hiding this comment.
This code is a perfect example of something that should live inside a selector and unit tested. Shoot, it could even go inside the getTimeMachineValues selector. Generally, anytime I'm doing a whole bunch of logic inside of mstp I like to ask myself "should this go into a selector?"
| @@ -48,7 +48,7 @@ const VariableToolbar: FunctionComponent<OwnProps & StateProps> = ({ | |||
| } | |||
|
|
|||
| const mstp = (state: AppState): StateProps => { | |||
| const variables = extractVariablesList(state) | |||
| const variables = extractVariablesListWithDefaults(state) | |||
There was a problem hiding this comment.
| const variables = extractVariablesListWithDefaults(state) | |
| const variables = getVariables(state) |
There was a problem hiding this comment.
I'd rather keep this as is since:
- We already have an oats generated API call by that name
- Default variables (like windowPeriod, timeRangeStart, timeRangeStop) are never returned when we currently deal with anything
getVariablerelated.
| @@ -53,3 +56,24 @@ describe('getTimeRangeVars', () => { | |||
| }) | |||
| }) | |||
| }) | |||
|
|
|||
| describe('getTimeRangeAsVariable', () => { | |||
asalem1
force-pushed
the
fix/display-variables
branch
from
88d42eb
to
5d7ba4e
Compare
| {dropdownItems.length === 1 && ( | ||
| <Dropdown | ||
| button={button} | ||
| style={{width: '200px'}} | ||
| menu={menu} | ||
| testID="variable--tooltip-dropdown" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Added this piece of logic after speaking to a user and them mentioning that dropdowns with only 1 option are confusing since they'd expect them to have more than 1 option
asalem1
force-pushed
the
fix/display-variables
branch
from
146d954
to
06b41dc
Compare
asalem1
force-pushed
the
fix/display-variables
branch
from
3016f07
to
8c05771
Compare
Closes #16058
Problem
Variable list was showing only created variables and not showing default variables
Solution
Default variables are populated whenever the query runs, so the solution involved calculating the default variables whenever the variable list was rendered & whenever the variable tooltip was being toggled. Here's how it looks like in action: