NEW Add ViewModeToggle component #560

raissanorth · 2018-07-04T02:31:13Z

This React component allows users to toggle between split, edit, and preview mode.

It utilises Reactstrap's ~~PopoverField~~ Dropdown, connects to a Redux store.

ViewModeToggle component:

ViewModeToggle component on hover:

Edit view mode chosen and split view mode enabled:

Edit view mode chosen and split view mode disabled:

Preview view mode chosen and hover on disabled split mode:

Sample of ViewModeToggle rendered within Preview component (split mode view chosen)

Sample of ViewModeToggle rendered within CampaignAdminList component

Important: The test cases introduce a dependency on the JS testing utility Enzyme (recommended by React for shallow rendering, see https://reactjs.org/docs/test-utils.html#overview)

A couple of other things to note:

~~The value for the minimum screen width to enable split view mode is derived from the CSS variable media-breakpoint-down(md) and currently hard coded in viewModeReducer.js~~ The value for the minimum screen width is based on the prior CSS variable media-breakpoint-down(md) used in the media query, minus the width of the expended cms-panel. It is stored in the newly added constants.js file and is used in the viewModeReducer.js
Rather than using Reactstrap's ToolTip component, this implementation using the same div and styling as the current PHP implementation of the toggle. As a consequence the height for of 37px for this div is hard coded to 'disable' the split view option when the screen width is too narrow. While the first approach is preferable, i.e. using the button's disable property in combination with Reactstrap's ToolTip, no events are fired on the disabled button which is why the latter approach was chosen.
Within ViewModeToggle-test.js the creation of the shallow rendered wrapper is repeated for each test group. To avoid unnecessary repetition, , the wrapper could be defined outside of the test groups. In this case the props need to be defined before the wrapper is created. As the tests use rely on different prop values, this would need to be changed within the inidividual tests.

robbieaverill · 2018-07-04T02:45:14Z

Added feedback-required/core-team (@silverstripe/core-team) since we're introducing a new React testing pattern with enzyme. This has been discussed already with @lukereative @chillu and @raissanorth internally at SilverStripe.

robbieaverill

Awesome work! I've left a couple of minor comments.

It also looks like your IDE isn't respecting the editorconfig rules (2 spaces for indentation) - perhaps you could tweak this so it's consistently using it?

robbieaverill · 2018-07-04T02:46:54Z

client/src/components/ViewModeToggle/ViewModeToggle.js

+    let classNames = 'btn btn-secondary icon-view last font-icon-edit-write';
+    if (activeState === 'edit') {
+      classNames += ' viewMode-selected';
+    }


We can use the classnames library for this, it's already part of admin

robbieaverill · 2018-07-04T02:52:43Z

client/src/components/Preview/Preview.js

@@ -123,6 +125,7 @@ class Preview extends Component {
          <div className="btn-toolbar">
            {this.renderBackButton()}
            {this.buildToolbarButtons()}
+            <ViewModeToggle id="view-mode-toggle-in-preview-nb" area={'preview'} />


Ideally we'd use the injector to inject a ViewModeToggle prop into Preview which would allow devs to customise this component in the Preview implementation if they wanted to. For example:

import { inject } from 'lib/Injector'; // ... render() { const { ViewModeComponent } = this.props; return ( // .. <ViewModeComponent { ...props } /> // ... Preview.propTypes = { ViewModeComponent: PropTypes.oneOfType([PropTypes.node, PropTypes.func]).isRequired, }; // ... export default inject( ['ViewModeToggle'], (ViewModeToggle) => ({ ViewModeComponent: ViewModeToggle }), 'Admin.Preview' );

You'll also need to register the component with Injector in src/boot/registerComponents.js

robbieaverill · 2018-07-04T02:54:04Z

client/src/styles/bundle.scss

@@ -59,6 +59,8 @@
 @import "../components/Toolbar/Toolbar";
 @import "../components/TreeDropdownField/TreeDropdownField";
 @import "../components/UsedOnTable/UsedOnTable";
+@import "../components/IframeDialog/IframeDialog";


Is this referenced somewhere else?

robbieaverill · 2018-07-04T02:55:32Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+  background: rgba(255, 255, 255, 0);
+
+  &::before {
+    content: "Screen size too small";


Is there any way of adding this with javascript or PHP? We can't make this string translatable if it's in CSS

Rather than having a :before pseudo element, you could have a span styled the same and injected to the .disabled-tooltip div element, this will enable translatable strings and not require any hacks.

If you want the hacked version, use the attr() css function and pick an arbitrary attribute to get the translation string from.

flamerohr

I glazed over the test file, I'll review it when I get more time
Overall it's a decent implementation - the main blocker for me is that the "tooltip" for disabled options could be improved.

flamerohr · 2018-07-09T10:17:30Z

client/src/components/ViewModeToggle/ViewModeToggle.js

+import { selectEditMode, selectPreviewMode, selectSplitMode } from 'state/viewMode/viewModeActions';
+import classNames from 'classnames';
+
+/* global window */


is there a need to declare the window global anymore?

flamerohr · 2018-07-09T10:22:06Z

client/src/components/ViewModeToggle/ViewModeToggle.js

+          case 'preview':
+              return 'font-icon-eye';
+          default:
+              return 'font-icon-columns';


the indent for these lines seem off, they're 4 spaces instead of 2.
Could you have a map of the classes somewhere? possibly provided through props, or another extensible means. It would make customising the icons by an external author possible.

flamerohr · 2018-07-09T10:22:34Z

client/src/components/ViewModeToggle/ViewModeToggle.js

+          case 'preview':
+              return i18n._t('Admin.PREVIEW_MODE', 'Preview mode');
+          default:
+              return i18n._t('Admin.SPLIT_MODE', 'Split mode');


Similar with the above comment

flamerohr · 2018-07-09T10:28:24Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+  background: rgba(255, 255, 255, 0);
+
+  &::before {
+    content: "Screen size too small";


Rather than having a :before pseudo element, you could have a span styled the same and injected to the .disabled-tooltip div element, this will enable translatable strings and not require any hacks.

If you want the hacked version, use the attr() css function and pick an arbitrary attribute to get the translation string from.

flamerohr · 2018-07-09T10:31:51Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+    display: none;
+    background: #555;
+    left: 47%;
+    top: -6px;


if you implement the span suggestion above, you can then use borders to implement the "speech bubble" effect which will reduce a lot of this juggling around with css3 rotate and other specific pixel placements.

flamerohr · 2018-07-09T10:32:37Z

client/src/state/viewMode/viewModeActions.js

+export function selectEditMode() {
+    return {
+        type: ACTION_TYPES.SELECT_EDIT,
+        payload: null,


can just omit the payload for all these actions...
Also indent is all over the place for this file

flamerohr · 2018-07-09T10:33:19Z

client/src/state/viewMode/viewModeActions.js

+export function enableOrDisableSplitMode(panelWidth) {
+  return {
+    type: ACTION_TYPES.SPLIT_AVAILABLE,
+    panelWidth


put panelWidth in a payload object:

{ type: ACTION_SOMETHING, payload: { panelWidth }, };

flamerohr · 2018-07-09T10:38:43Z

I remember silverstripe-admin had a click simulator somewhere... maybe it was removed because it didn't prove useful.
If Enzyme helps, then got for it. However, I think it's important that you keep it in devDependencies instead of dependencies

raissanorth · 2018-07-11T05:13:32Z

Hi @flamerohr, thanks a lot for your feedback. I have addressed most things you mentioned - as well as changed the component to use Dropdown instead of PopoverField @lukereative. Unfortunately, yarn throws an error when I try to add enzyme to the dev-dependencies:

raissanorth · 2018-07-11T21:42:58Z

If approved I suggest we implement this in https://github.com/silverstripe/silverstripe-versioned-admin. A PR was made to silverstripe/silverstripe-campaign-admin#109 already.

flamerohr · 2018-07-11T23:40:15Z

If approved I suggest we implement this in https://github.com/silverstripe/silverstripe-versioned-admin.

That would become a weird circular dependency with the Preview component becoming dependent on ViewModeToggle which belongs to a module which depends on this module.

Does injector return null if a component isn't registered? I remember seeing an error thrown.

Unfortunately, yarn throws an error when I try to add enzyme to the dev-dependencies.

That's a shame, you can add // eslint-disable import/no-extraneous-dependencies to the top of your test file so you can leave it in dev-dependencies...
I think there was a way to config eslint to ignore import dependencies for test files, but the above comment works as a temporary measure to get this through

robbieaverill · 2018-07-11T23:41:46Z

I don't think that means move the component to versioned-admin, but add the view mode toggle to history viewer in versioned-admin when merged here

flamerohr

Approved with the condition of moving enzyme to dev-dependencies

flamerohr · 2018-07-11T23:44:56Z

I don't think that means move the component to versioned-admin, but add the view mode toggle to history viewer in versioned-admin when merged here

Sweet, that makes more sense

robbieaverill

Looking great! Super minor comments

robbieaverill · 2018-07-12T00:08:10Z

client/src/components/ViewModeToggle/ViewModeToggle.js

+        dropdownMenuProps
+      } = this.props;
+      let { className } = this.props;
+      className = classNames(className);


I think this is a little confusing - firstly we dereference a prop then we reassign it, unclear as to whether this reassigns the prop which we shouldn't really be doing, I assume it doesn't but only the local variable. Could you instead create a const dropdownClassName = classNames(className); maybe?

robbieaverill · 2018-07-12T00:09:27Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+  left: 0;
+  right: 0;
+  height: 37px;
+  background: rgba(255, 255, 255, 0);


I think you could probably use some variables for this, and for the #555 lower. See $gray-darker etc in variables.scss and maybe use darken($gray-dark, 10%) if you need to

robbieaverill · 2018-07-12T00:09:47Z

client/src/lib/constants.js

@@ -0,0 +1 @@
+export const SPLITMODE_BREAKPOINT = 800;


It'd be good to add a comment describing what this is for too :)

I'd love to see this configurable somehow (maybe stored in the redux store instead), but out of scope for this pull request I think

robbieaverill · 2018-07-12T00:09:55Z

client/src/state/viewMode/viewModeActionTypes.js

+    SELECT_EDIT: 'SELECT_EDIT',
+    SELECT_PREVIEW: 'SELECT_PREVIEW',
+    SELECT_SPLIT: 'SELECT_SPLIT',
+    SPLIT_AVAILABLE: 'SPLIT_AVAILABLE',


Indentation is out here

raissanorth · 2018-07-12T21:22:22Z

~~Changed the status to WIP since there are some minor design changes, discussed with @clarkepaul yesterday, still to be made.~~

Added all requested styling improvements.

This React component allows users to toggle between split, edit, and preview mode. It utilises Reactstrap's Dropdown, connects to a Redux store, and its test cases introduce a dependency on the JS testing utility Enzyme.

robbieaverill

Some last requests for SCSS reusability =)

We could write tests for the reducer too, but it's not totally necessary and would only really be for the ACTION_TYPES.SPLIT_AVAILABLE case

robbieaverill · 2018-07-17T03:08:48Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

@@ -0,0 +1,122 @@
+.viewmode-dropdown-toggle {
+  margin-left: auto;
+  font-size: 13px;


You could use $font-size-base here

robbieaverill · 2018-07-17T03:09:36Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+  }
+
+  .viewModeToggle-chosenView-title {
+    font-size: 10px;


$font-size-xxs

robbieaverill · 2018-07-17T03:10:55Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+
+  .viewModeToggle-chosenView-title {
+    font-size: 10px;
+    line-height: 10px;


If there's an appropriate line-height variable in _variables.scss or _bootstrap-variables.scss in admin that'd be ideal to use

robbieaverill · 2018-07-17T03:11:45Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+
+    .viewmode-dropdown-toggle {
+      &:before {
+        font-size: 18px;


$h3-font-size: 17px or $h2-font-size: 19px - if you could use one of those we'd be reducing customisation away from core

robbieaverill · 2018-07-17T03:13:40Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+        box-shadow: inset 0 3px 5px rgba(0, 0, 0, 0.125), 0 0 0 0.2rem rgba(86, 107, 141, 0.5);
+        border-color: #e7ebf0;
+        background-color: transparent;
+        color: #566b8d;


Is there any way we can swap these out for variables, or even darken() or lighten() calls on variables that are close to these values already?

robbieaverill · 2018-07-17T03:13:52Z

client/src/components/ViewModeToggle/ViewModeToggle.scss

+}
+
+
+


Just one empty line at the end would be cool

robbieaverill

Looks awesome!

robbieaverill added type/enhancement affects/v4 change/minor impact/medium labels Jul 4, 2018

robbieaverill suggested changes Jul 4, 2018

View reviewed changes

raissanorth mentioned this pull request Jul 6, 2018

Add split/edit/preview toggle component to work with the Preview component #480

Closed

3 tasks

raissanorth force-pushed the pulls/1/build-view-mode-toggle-component branch from 9bc8727 to 4ab0615 Compare July 9, 2018 02:45

flamerohr suggested changes Jul 9, 2018

View reviewed changes

raissanorth changed the title ~~NEW Add ViewModeToggle component~~ [WIP] NEW Add ViewModeToggle component Jul 9, 2018

raissanorth force-pushed the pulls/1/build-view-mode-toggle-component branch from 4ab0615 to a255c32 Compare July 11, 2018 05:09

raissanorth mentioned this pull request Jul 11, 2018

NEW Make use of ViewModeToggle component. silverstripe/silverstripe-campaign-admin#109

Merged

raissanorth changed the title ~~[WIP] NEW Add ViewModeToggle component~~ NEW Add ViewModeToggle component Jul 11, 2018

flamerohr approved these changes Jul 11, 2018

View reviewed changes

robbieaverill reviewed Jul 12, 2018

View reviewed changes

raissanorth force-pushed the pulls/1/build-view-mode-toggle-component branch from a255c32 to 6b14802 Compare July 12, 2018 04:33

raissanorth changed the title ~~NEW Add ViewModeToggle component~~ WIP NEW Add ViewModeToggle component Jul 12, 2018

raissanorth force-pushed the pulls/1/build-view-mode-toggle-component branch 2 times, most recently from a31c6d5 to f5d203f Compare July 16, 2018 00:16

raissanorth changed the title ~~WIP NEW Add ViewModeToggle component~~ NEW Add ViewModeToggle component Jul 16, 2018

raissanorth force-pushed the pulls/1/build-view-mode-toggle-component branch from f5d203f to 9394667 Compare July 16, 2018 23:02

NEW Add ViewModeToggle component

5108734

This React component allows users to toggle between split, edit, and preview mode. It utilises Reactstrap's Dropdown, connects to a Redux store, and its test cases introduce a dependency on the JS testing utility Enzyme.

raissanorth force-pushed the pulls/1/build-view-mode-toggle-component branch from 9394667 to 5108734 Compare July 17, 2018 03:11

robbieaverill suggested changes Jul 17, 2018

View reviewed changes

robbieaverill removed the feedback-required/core-team label Jul 17, 2018

Use variables instead of hardcoded values for styling ViewModeToggle

52a1097

robbieaverill approved these changes Jul 17, 2018

View reviewed changes

robbieaverill merged commit 792002f into silverstripe:1 Jul 17, 2018

robbieaverill deleted the pulls/1/build-view-mode-toggle-component branch July 17, 2018 04:43

This was referenced Sep 19, 2018

Editor "TinyMCE" reduce UI impact, better use in smaller areas #653

Open

Consider the space usage for forms in small spaces #655

Open

NEW Add ViewModeToggle component #560

NEW Add ViewModeToggle component #560

Conversation

raissanorth commented Jul 4, 2018 • edited Loading

robbieaverill commented Jul 4, 2018

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flamerohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flamerohr commented Jul 9, 2018

raissanorth commented Jul 11, 2018

raissanorth commented Jul 11, 2018

flamerohr commented Jul 11, 2018

robbieaverill commented Jul 11, 2018

flamerohr left a comment

Choose a reason for hiding this comment

flamerohr commented Jul 11, 2018

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raissanorth commented Jul 12, 2018 • edited Loading

robbieaverill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbieaverill left a comment

Choose a reason for hiding this comment

raissanorth commented Jul 4, 2018 •

edited

Loading

raissanorth commented Jul 12, 2018 •

edited

Loading