Add full-screen code functionality to and remove custom theming from EuiCodeBlock by cjcenizal · Pull Request #259 · elastic/eui

cjcenizal · 2018-01-03T19:51:00Z

This change also removes the .euiCodeBlock--dark and .euiCodeBlock--light classes. If you have CSS selectors which depend upon these classes then your <EuiCodeBlock> instances may appear broken. To reinstate the styles applied by those selectors, you'll have to apply those color values as SCSS color variables.

snide

Cool. Think we should just add this to the component itself? Seems useful enough to be an optional prop allowFullscreen on EuiCodeBlock?

Stylistically I can clean it up a bit in a separate PR. Functionally I'd add a keydown event for ESC to close it, otherwise works as advertised.

cjcenizal · 2018-01-06T00:44:37Z

Good idea @snide! I'll add that.

cjcenizal · 2018-01-06T02:18:34Z

@snide I made a few changes. Could you take a look?

Added full-screen functionality to all code blocks.
I changed the way highlighting works so that it adheres to the active theme. I think this makes the most sense in terms of how highlighting colors and theme colors interact.
Made a few small adjustments to the way the EuiImage code is formatted.

snide

Added full-screen functionality to all code blocks.

Think something is up with your focus-trap / ESC combo. Wasn't able to get either working correctly. I could still tab through the original document. That stuff is tricky.

I'll clean up the styling a bit in a separate PR after you've got it in. Off-hand I'll likely make the icon absolutely positioned so it's always around as you scroll, make the coloring a little more monotone and only show the icon on hover of the block.

Made a few small adjustments to the way the EuiImage code is formatted.

Looks good. All your changes are great.

I changed the way highlighting works so that it adheres to the active theme. I think this makes the most sense in terms of how highlighting colors and theme colors interact.

I ... am ok with this, but I'll likely go in after the fact and clean up that light theme. Probably add a border around the block...etc. I think you'll want to ping @bevacqua just to let him know, since I think he was utilizing some theming overwrites and will need to make some changes to cloud.

snide · 2018-01-06T03:43:06Z

src/components/code/_code_block.scss

+    left: 0;
+    right: 0;
+    bottom: 0;
+    padding: $euiSizeL !important;


I'd bump this up a notch since you have so much space to work with.

snide · 2018-01-06T03:44:28Z

src/global_styling/variables/_colors.scss

+
+// Code
+
+$euiCodeBlockBackgroundColor: #F5F5F5 !default;


This is just $euiColorLightestShade

snide · 2018-01-06T03:45:35Z

src/global_styling/variables/_colors.scss

+// Code
+
+$euiCodeBlockBackgroundColor: #F5F5F5 !default;
+$euiCodeBlockColor: #3F3F3F !default;


This is just $euiTextColor. Know these seems small, but saves people from needed to edit multiple vars.

Thanks good catch.

snide · 2018-01-06T03:46:36Z

src/components/image/image.js

      <figure
        className={classes}
-        onClick={this.toggleImageFullScreen}
+        onClick={allowFullScreen ? this.openFullScreen : undefined}


TY. This is a better way to do this.

snide · 2018-01-06T03:53:19Z

CHANGELOG.md

-No public interface changes since `0.0.11`.
+**Breaking changes**
+
+- Removed `color` prop from `<EuiCodeBlock>`. This component's highlighting now matches whichever theme is currently active. [(#259)](https://github.com/elastic/eui/pull/259)


When we do a breaking change like this, can we make it a policy to put upgrade instructions in the PR itself. I think I did that for the last couple, and if not I should be! This ones funny though in that it won't technically break, it should just stylistically change.

I totally agree. Should I just add a note that this may result in a stylistic shift but no change in functionality, since color is now just a no-op? I figure that people will know to remove the use of this prop now that it no longer exists, but I can mention that too if you think we need to be that explicit.

Yeah. I think we just check on each other when we make merges to add something like the below...

Upgrade path

This PR only changes the look of previous code blocks and does not require any changes if you are OK with the new styling. It will however break styling if you were applying custom code block themes beyond the default values. To retain that custom coloring, you'll want to apply those color values to the colors.scss files directly now.

Ahhh I think I get it! You mean we need to update the CHANGELOG with fallout caused by breaking changes to the SCSS, too right? I completely overlooked that, so thanks for pointing it out. I just pushed an update, let me know if I need to adjust it.

cjcenizal · 2018-01-07T01:36:45Z

@snide Thanks for the solid feedback. I addressed your notes, could you take another look? I also added Nico as a reviewer, thanks for that suggestion. Your style improvements sound great. This PR was pretty quick-and-dirty from a design standpoint.

…ly active.

cjcenizal · 2018-01-09T20:03:53Z

@uboness You can rebase master to get the full screen code example functionality now.

cjcenizal requested review from snide and uboness January 3, 2018 19:51

snide reviewed Jan 3, 2018

View reviewed changes

cjcenizal force-pushed the docs/full-screen-code branch from e344576 to 78e1936 Compare January 6, 2018 02:16

cjcenizal force-pushed the docs/full-screen-code branch from e6b5d87 to 531fe78 Compare January 6, 2018 02:20

snide suggested changes Jan 6, 2018

View reviewed changes

snide reviewed Jan 6, 2018

View reviewed changes

snide added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Jan 6, 2018

cjcenizal requested a review from bevacqua January 7, 2018 01:34

snide approved these changes Jan 7, 2018

View reviewed changes

cjcenizal changed the title ~~Add full-screen code example functionality.~~ Add full-screen code example functionality and default theming. Jan 8, 2018

cjcenizal changed the title ~~Add full-screen code example functionality and default theming.~~ Add full-screen code example functionality and remove custom theming. Jan 8, 2018

cjcenizal changed the title ~~Add full-screen code example functionality and remove custom theming.~~ Add full-screen code functionality to and remove custom theming from EuiCodeBlock Jan 8, 2018

cjcenizal force-pushed the docs/full-screen-code branch from f9f74aa to 26e79c1 Compare January 9, 2018 19:36

cjcenizal added 8 commits January 9, 2018 11:37

Add full-screen code example functionality.

ac44a6c

Remove color prop from EuiCodeBlock. Match whichever theme is current…

5065050

…ly active.

Slight tweaks to EuiImage.

f6ba623

Add full-screen functionality to EuiCodeBlock.

6becba1

Undo changes to GuideSection.

ee4a391

Remove GuideCode styles.

8bd33c9

Improve use of color variables and add more padding.

cb8a02c

Fix ESC key handler.

2e4771d

cjcenizal force-pushed the docs/full-screen-code branch from 26e79c1 to 2e4771d Compare January 9, 2018 19:37

Fix positioning of expand/collapse button.

3310f54

cjcenizal merged commit 165fb28 into elastic:master Jan 9, 2018

cjcenizal deleted the docs/full-screen-code branch January 9, 2018 20:03

snide mentioned this pull request Jan 18, 2018

Code editor themes should be opinionated #152

Closed

Conversation

cjcenizal commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Jan 6, 2018

Uh oh!

cjcenizal commented Jan 6, 2018

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Upgrade path

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Jan 7, 2018

Uh oh!

cjcenizal commented Jan 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cjcenizal commented Jan 3, 2018 •

edited

Loading

cjcenizal Jan 7, 2018 •

edited

Loading