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

EUIFication: Grok Debugger #20027

Merged

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Jun 19, 2018

This PR converts the Grok Debugger code to use React and EUI components. There should be no functional changes to the Grok Debugger as a result of this PR.

Before this PR

before

After this PR

after

Testing this PR

The functional tests for this feature haven't changed in this PR (except a small change to how the Ace editor elements are selected) but if you want to test things manually, you just need to make sure that the following features work the same in this PR as they did before (in master):

  • Grok patterns can be simulated, including when using custom grok patterns.
  • When entering the grok pattern in the "Grok Pattern" field, syntax highlighting works.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ycombinator ycombinator force-pushed the dev-tools/grokdebugger/eui-fication branch from bf395b2 to bcfadf0 Compare June 21, 2018 23:22
@ycombinator ycombinator added review and removed WIP Work in progress labels Jun 21, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM! Just had some UI suggestions. I've reviewed the code and taken a look at this in the browser, but I don't know how to test the functionality to verify that nothing has changed. If you'd like me to do that, could you provide some steps?

return (
<EuiFormRow
label="Structured Data"
data-test-subj="aceEventOutput"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to add fullWidth here to make it expand to 100% width of the container.

label="Structured Data"
data-test-subj="aceEventOutput"
>
<EuiCodeEditor
Copy link
Contributor

Choose a reason for hiding this comment

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

How about wrapping this in EuiPanel? Same for PatternInput, EventInput, and the code editor in CustomPatternsInput.

      <EuiPanel paddingSize="s">
        <EuiCodeEditor
          mode="json"
          isReadOnly
          width="100%"
          height="340px"
          value={JSON.stringify(value, null, 2)}
          setOptions={{
            highlightActiveLine: false,
            highlightGutterLine: false,
          }}
        />
      </EuiPanel>

Then it will look like this:

image

buttonContent="Custom Patterns"
data-test-subj="btnToggleCustomPatternsInput"
>
<EuiCallOut
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a <EuiSpacer size="m" /> above this call out? This will give it a bit of breathing room:

image

@ycombinator ycombinator force-pushed the dev-tools/grokdebugger/eui-fication branch from 9f5f05e to 68e948f Compare June 22, 2018 13:03
@ycombinator
Copy link
Contributor Author

@cjcenizal Thanks for the UI feedback; I've made the changes you suggested and the UI looks much better! I've also updated the PR description with more-detailed test steps. Finally, I made some small code changes in 61faa84 and 68e948f to handle clearing out of state better.

Ready for your 👀 again.

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

LGTM - great work!

EDIT: pending CI passing ;)


const app = uiModules.get('xpack/grokdebugger');

app.directive('grokdebugger', function ($injector) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Love how much less Angular I'm seeing in this PR!

@ycombinator
Copy link
Contributor Author

CI is failing on Grok Debugger functional tests; I'm working on updating those.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

🔥 LGTM!

@ycombinator ycombinator merged commit da2da74 into elastic:master Jun 22, 2018
ycombinator added a commit that referenced this pull request Jun 22, 2018
* Move <kbn-dev-tools-app> Angular wrapper directive usage into route template

* Move sole directive into new directives folder

* WIP checkin: basic conversion to React components

* Fix sample custom patterns indentation

* Remove custom CSS

* Wrap button in EuiFormRow + define isSimulateDisabled

* Wire up simulate

* Cleanup

* Ace formatting options

* Better styling

* Adding spacing between custom patterns and simulate button

* Fixing form row widths

* Removing form row around button

* Add indentation/newlines in structured output

* Error handling

* Use constants

* Removing no-longer-used code

* Implement syntax highlighting via custom mode

* Making functional tests pass

* Adding trailing comma back

* Removing fixed heights

* Removing unnecessary styles

* Make Event Output form row full width as well

* Wrapping EuiCodeEditors in EuiPanels

* Adding spacer before callout; making spacing around callout consistent

* Clear out custom patterns from request if field is cleared out

* Clear out simulation results before attempting simulation

* Set state with untrimmed value
@ycombinator
Copy link
Contributor Author

Backported to:

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

Successfully merging this pull request may close these issues.

4 participants