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

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

Merged
merged 2 commits into from
May 12, 2020

Conversation

alexwizp
Copy link
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

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
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
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
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
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