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

[Lens] Data panel styling and optimizations #40787

Open
wants to merge 102 commits into
base: feature/lens
from

Conversation

Projects
None yet
5 participants
@chrisdavies
Copy link
Contributor

commented Jul 10, 2019

Summary

A number of things are bundled up in this PR. It's Joe's styling PR, with optimization tweaks and a few bug fixes.

image

In addition to Joe's original PR, I:

  • Added debouncing to the areas that render via expressions
  • Memoized the dragging state so that memoized React components don't constantly re-render
  • Tweaked some styles for Firefox
  • Fixed rendering of horizontal bar
  • Simplified the way highlighting of search term is done
    • The previous implementation was functionally better, but caused slowdown on large indices
    • The current implementation only highlights the first match in a field name, rather than all the matches
  • Memoized the result of getPotentialColumns, which was pretty expensive to run on each render... we could probably optimize this further, to be computed only once per index pattern, but for now, it's sufficient

wylieconlon and others added some commits May 30, 2019

@cchaos

cchaos approved these changes Jul 10, 2019

Copy link
Contributor

left a comment

Quick glance at SASS is fine.

margin: 0 $euiSize;
}

& > h4 {

This comment has been minimized.

Copy link
@cchaos

cchaos Jul 10, 2019

Contributor

I'm not too worried about the exact SASS usage right now, but you might want to stay away from these generic selectors as that element could easily change. Best to stick with classes.

This comment has been minimized.

Copy link
@chrisdavies

chrisdavies Jul 11, 2019

Author Contributor

Yah. Agreed. Also, the specificity of this selector is really high.

chrisdavies added some commits Jul 11, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

chrisdavies added some commits Jul 11, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

@wylieconlon
Copy link
Member

left a comment

I think this is still incomplete, I think some of my previous comments haven't been responded to. I am hoping that one more round of reviews will be enough to merge.

await new Promise(r => setTimeout(r, 1));
expect(component.html()).toMatchInlineSnapshot(`"<h1>yall</h1>"`);
});
});

This comment has been minimized.

Copy link
@wylieconlon

wylieconlon Jul 12, 2019

Member

The reason I say they're weak is that snapshots aren't testing the behavior of the system. If I change the behavior here, I want tests to fail. Instead of tests failing with a specific message, it says that the snapshots have changed and I see a diff that I have to interpret myself. There's no assertion of specific behavior.

For example, the issue with the SeriesType in the XY chart was made worse by snapshot testing. We were snapshotting the rendered chart, but without any assertions that the right series was rendered. So until I fixed it, we were rendering the horizontal_bar chart as a vertical area chart.

My preferred solution is combination, like I did for the SeriesType issue:

    test('it renders stacked horizontal bar', () => {
      const { data, args } = sampleArgs();

      const component = shallow(
        <XYChart data={data} args={{ ...args, seriesType: 'horizontal_bar_stacked' }} />
      );
      expect(component).toMatchSnapshot();
      expect(component.find(BarSeries)).toHaveLength(1);
      expect(component.find(BarSeries).prop('stackAccessors')).toHaveLength(1);
      expect(component.find(Settings).prop('rotation')).toEqual(90);
});

That kind of test will catch both unexpected additions and fail fast if the required behavior changes.

@@ -64,7 +64,7 @@ export function RootDragDropProvider({ children }: { children: React.ReactNode }
const [state, setState] = useState<{ dragging: unknown }>({
dragging: undefined,
});
const setDragging = (dragging: unknown) => setState({ dragging });
const setDragging = useMemo(() => (dragging: unknown) => setState({ dragging }), [setState]);

This comment has been minimized.

Copy link
@wylieconlon

wylieconlon Jul 12, 2019

Member

Something like:

expect(component1.prop('setDragging')).toEqual(component2.prop('setDragging'))
setShowIndexPatternSwitcher(false);
props.setState({
...props.state,
currentIndexPatternId: choices[0].value as string,

This comment has been minimized.

Copy link
@wylieconlon

wylieconlon Jul 12, 2019

Member

I think this is an 80/20% case- which is more convenient in the 80% case? I think your example is a 20% case and implies that every time a user switches indexes they will have to manually deselect all filter types and also clear any text they've typed.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@chrisdavies

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@wylieconlon I think this is mostly good. I'd like to test the drag / drop provider, but didn't have time to get to that today, so I'll be making one more update to this sometime tomorrow.

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@wylieconlon
Copy link
Member

left a comment

Changes LGTM!

jest.useFakeTimers();

describe('RootDragDropProvider', () => {
test('provides a consistent context across render cycles', () => {

This comment has been minimized.

Copy link
@wylieconlon

wylieconlon Jul 16, 2019

Member

The contents of this test are fine, but maybe the description here could be updated to something like "reuses contexts for each render"?

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

@chrisdavies

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

retest

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.