Updated code mirror component for consistency #11500

arnav28 · 2021-04-29T22:05:07Z

Hide gutters, line number and selection while read only
Show toolbar with copy functionality for all instances

- Hide gutters, line number and selection while read only - Show toolbar with copy functionality for all instances

ivana-mcconnell · 2021-04-30T15:04:09Z

Would it be possible to see an example where there's help text included?

hashishaw

A few things to revise, and since the previous component was just an extension of a third party library we didn't have any tests for it. We should add tests for this one since we're doing custom logic (esp to make sure the options join with the defaults correctly, show or don't show the toolbar, etc)

hashishaw · 2021-04-30T15:55:58Z

ui/app/components/json-editor.js

-      }, this);
+  init() {
+    this._super(...arguments);
+    this.options = { ...JSON_EDITOR_DEFAULTS, ...this.options };


Are you trying to set it to the passed in options here? That would be this.args.options I believe

options is a param on this component. Line 25 will merge the object properties from options passed in the child component with the default ones defined in JSON_EDITOR_DEFAULTS.

Oh my bad, I was thinking this was a glimmer component 👍

hashishaw · 2021-04-30T15:59:42Z

ui/app/templates/components/secret-edit-display.hbs

-        Secret data
-        {{/if}}
-      </label>
+      <Toolbar>


Could we have this toolbar as part of this new component, maybe an optional piece? Or it could be a separate component <ToolbarJsonEditor> or something. That will make it easier to keep the behavior and look consistent between instances

I did that but later realized we have a comment with an additional action for the upload file as well. Need to come up with a better design so we can create a separate ToolbarJsonEditor component with support for custom actions.

Since the actions for each button will need to be passed in anyway, you could have the JSON component take "children" that get yielded within the toolbar. Or, if we know we want a discreet number of options (probably not) we could to a contextual component. Passing in the yielded content would be simpler, but it does depend on the developer using the correct class names for consistency in styling

Done. Moved the Toolbar to json-editor component itself and added a yield block for custom actions.

hashishaw · 2021-04-30T16:01:56Z

ui/app/templates/components/transit-key-action/hmac.hbs

+        <JsonEditor
+            @valueUpdated={{action (mut input)}} 
+            @options={{hash
+              lineNumbers=true


On these instances where we're swapping out IvyCodemirror for the JsonEditor component, we can simplify the options passed since many of these are in the default settings for the JsonEditor component

Good point will fix it.

hashishaw · 2021-04-30T16:07:00Z

ui/lib/core/addon/templates/components/form-field.hbs

@@ -223,6 +241,7 @@
          theme=(or attr.options.theme 'hashi')
        }}
      />
+      <p class="sub-text">{{ attr.options.helpText }}</p>


We should only show this if attr.options.helpText exists. This also might error out if there are no options passed, so I would double check that

True, will add defensive checks.

arnav28 · 2021-05-04T23:26:04Z

Screen.Recording.2021-05-04.at.4.20.19.PM.mov

ivana-mcconnell · 2021-05-05T14:57:14Z

Thanks for the recording! I noticed a couple of things with the cursor hover behaviour. It's backwards in a couple of spots:

The hover on the "read" state is the text cursor at 0:05. The cursor's hover on the read state should be "default." This is also the case at 0:24, when viewing the policy. The cursor is "text" in the read state when hovering, and should be "default".
When looking at signing the SSH key (0:17), it seems like the cursor is "default" when hovering, but this is an edit state. In this case, the cursor should be "text"

Additionally, would it be possible to see an example of one of the code blocks with help text?

hashishaw

Looking great! I think we should just add one more test, and I'll pull it down and play with it in the meantime

hashishaw · 2021-05-05T16:18:49Z

ui/tests/integration/components/json-editor-test.js

+    assert.equal(component.canEdit, true, 'json editor is in read only mode');
+  });
+
+  test('it renders in read only mode', async function(assert) {


Can we add one more test that ensures the toolbar shows only when showToolbar = true?

ivana-mcconnell · 2021-05-05T20:53:20Z

After a chat with @arnav28 to clarify cursor behaviour, we've decided to leave them as they are.

The current behaviour is consistent with how the cursor behaves in the browser: when hovering over text, it's the text cursor. When it isn't hovering over text, the cursor is default. We'll keep this. The original reasoning for changing it was that users would get confused that the read-only code was editable. However, we've made other modifications which should help solve this problem. Those are:

Removing the 'flashing' cursor in the read-only mode. Previously, the user could click into the read-only version and see an 'editing' cursor (see image below). This gave the impression that editing was possible when it wasn't.
We've removed the line numbering in the read-only view, which also contributes to the assumption that the code is editable.

The hope is that these visual changes will be enough to prevent customers assuming that the read-only code component is editable. There's no need to change cursor behaviour. If there's still confusion, we can look at making more visual changes to the code display in read-only mode.

This looks good to me now!

hashishaw

Great work! 🚀

Updated code mirror component for consistency

f61c01f

- Hide gutters, line number and selection while read only - Show toolbar with copy functionality for all instances

arnav28 added the ui label Apr 29, 2021

arnav28 requested a review from hashishaw April 29, 2021 22:18

hashishaw suggested changes Apr 30, 2021

View reviewed changes

Moved toolbar and actions to json editor component

b17067f

vercel bot deployed to Preview – vault-storybook May 4, 2021 21:59 View deployment

vercel bot temporarily deployed to Preview – vault May 4, 2021 21:59 Inactive

Updated form-field-from-model template

4ef58ac

vercel bot deployed to Preview – vault-storybook May 4, 2021 23:23 View deployment

vercel bot temporarily deployed to Preview – vault May 4, 2021 23:23 Inactive

hashishaw reviewed May 5, 2021

View reviewed changes

Added test for toolbar

71e5ff9

vercel bot deployed to Preview – vault-storybook May 5, 2021 21:11 View deployment

vercel bot temporarily deployed to Preview – vault May 5, 2021 21:11 Inactive

hashishaw approved these changes May 6, 2021

View reviewed changes

arnav28 merged commit a068cca into master May 6, 2021

arnav28 deleted the ui/ivycodemirror branch May 6, 2021 16:59

trutled3 mentioned this pull request Aug 18, 2021

Unable to copy inline text from vault secret #12350

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated code mirror component for consistency #11500

Updated code mirror component for consistency #11500

arnav28 commented Apr 29, 2021

ivana-mcconnell commented Apr 30, 2021

hashishaw left a comment

hashishaw Apr 30, 2021

arnav28 May 3, 2021

hashishaw May 3, 2021

hashishaw Apr 30, 2021

arnav28 May 3, 2021

hashishaw May 3, 2021

arnav28 May 4, 2021

hashishaw Apr 30, 2021

arnav28 May 3, 2021

hashishaw Apr 30, 2021

arnav28 May 3, 2021

arnav28 commented May 4, 2021

ivana-mcconnell commented May 5, 2021

hashishaw left a comment

hashishaw May 5, 2021

arnav28 May 5, 2021

ivana-mcconnell commented May 5, 2021 •

edited

Loading

hashishaw left a comment

Updated code mirror component for consistency #11500

Updated code mirror component for consistency #11500

Conversation

arnav28 commented Apr 29, 2021

ivana-mcconnell commented Apr 30, 2021

hashishaw 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

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

arnav28 commented May 4, 2021

ivana-mcconnell commented May 5, 2021

hashishaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivana-mcconnell commented May 5, 2021 • edited Loading

hashishaw left a comment

Choose a reason for hiding this comment

ivana-mcconnell commented May 5, 2021 •

edited

Loading