Skip to content
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

fix(ui): disabled group functionality for check query builder #17113

Merged
merged 7 commits into from
Mar 6, 2020

Conversation

asalem1
Copy link
Contributor

@asalem1 asalem1 commented Mar 6, 2020

Closes #16956

Problem

Group functionality shouldn't exist on the query builder when setting up checks for alerts

Solution

Created custom logic to only display filter as an option and disable the dropdown to reduce functionality confusion

disable_group

  • CHANGELOG.md updated with a link to the PR (not the Issue)

Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a seemingly small change - what is being passed to down, but the implications of where the logic of this change lives are big for the future

testID="select-option-dropdown"
onSelect={onSelect ? onSelect : emptyFunction}
/>
{options.length === 1 && (
Copy link
Contributor

@hoorayimhelping hoorayimhelping Mar 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments on this block:

Generally: if you're going to do a statement of the form

if condition then foo
if !condition then bar

it's typically more common, and easier to follow an if/else

{options.length === 1 ? (<Dropdown />) : (<SelectDropdown />)}

the else is there to handle this specific case :)

Second: This method is inferring what state to put itself into based on, well an inference, rather than checking values or the state of the app.

The length of the options array being 1 may indicate that we should only show the filter, but it may also indicate something else. The issue here isn't hiding group when we're in the checks overlay, it's hiding group when the options list has one item in it - we may only want to show group, but the inference is that if there's only a single item, it must show filter.

What we're interested in is whether this is happening in the checks overlay, right? So how about passing isInCheckOverlay as a prop down to BuilderCardDropdownHeader - if the prop is present and true, only show filter, otherwise show group and filter.

I know it seems persnickety, but these small things add up in a codebase. I prefer this because it moves the logic into a single place, and it makes it clear, in context, what the intent is - you don't have to look through two files to see what's happening. It also concentrates the logic - rather than spreading the logic to be implicit in the length of the array, which is set based on whether we're in the check overlay, it instead renders a different control based on whether we're in the check overlay. It's like a 'one hop' logic resolution, versus a 'two hop' one with the options array.

asalem1 and others added 3 commits March 6, 2020 09:27
Co-Authored-By: Bucky Schwarz <hoorayimhelping@users.noreply.github.com>
Co-Authored-By: Bucky Schwarz <hoorayimhelping@users.noreply.github.com>
@asalem1 asalem1 merged commit ddc0d9a into master Mar 6, 2020
@asalem1 asalem1 deleted the fix/disable-group branch March 6, 2020 18:46
jacobmarble pushed a commit that referenced this pull request Mar 12, 2020
fix(ui): disabled group functionality for check query builder
@gueluelue
Copy link

Is the group functionality not intended for checks or is it just currently not implemented correctly and thus turned off via the UI?

@asalem1
Copy link
Contributor Author

asalem1 commented Mar 20, 2020

Hi @gueluelue thanks for the question. It's actually somewhere in between where the functionality was not intended for checks at the moment, and so it could not be implemented correctly for checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable group by in query builder in check query builder
3 participants