Skip to content

[EuiCodeEditor] Console error when using the editor without import the default theme#3454

Merged
alexwizp merged 2 commits intoelastic:masterfrom
alexwizp:code_editor
May 12, 2020
Merged

[EuiCodeEditor] Console error when using the editor without import the default theme#3454
alexwizp merged 2 commits intoelastic:masterfrom
alexwizp:code_editor

Conversation

@alexwizp
Copy link
Copy Markdown
Contributor

Summary

In #2970 the default value for the theme was set to github. As a result, now we have many places in Kibana where we need to import this theme even if we don't want to specify theme (remind you it's optional property).

Related issue in Kibana repo:
elastic/kibana#56424

I think if we decided to set github as a default theme, it should be loaded here

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@alexwizp alexwizp requested review from cchaos and thompsongl May 11, 2020 10:25
@alexwizp alexwizp self-assigned this May 11, 2020
@kibanamachine
Copy link
Copy Markdown

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@alexwizp alexwizp marked this pull request as ready for review May 11, 2020 10:26
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Thanks for the background & fix!

Can you please add a bugfix changelog entry to CHANGELOG.md referencing this PR? After that, this will be good to merge

Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

After a slack conversation with the team, instead of defaulting to github we should go back to passing an undefined theme by default - EUI applies some custom style rules in our CSS build, rather than through brace's theme configuration.

@alexwizp
Copy link
Copy Markdown
Contributor Author

@chandlerprall is not sure we should pass undefined because this will return the problem of why the default value was added. My suggestion is to set the textmate theme, because this is the default theme for the Brace editor

@alexwizp alexwizp requested a review from chandlerprall May 12, 2020 10:17
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

The previous PR's goal was adding documentation around the theme prop, changing the default was secondary and probably wasn't the right thing to merge with. I like your update to textmate, and confirmed everything locally runs without errors when using only those defaults.

undefined would be fine as textmate is still the underlying default, but being explicit here helps readability

@alexwizp alexwizp merged commit 4ad4f2e into elastic:master May 12, 2020
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.

3 participants