-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI: Allow repeat data wrapping for wrap tool #27289
Conversation
CI Results: |
@onClear={{action "onClear"}} | ||
@codemirrorUpdated={{action "codemirrorUpdated"}} | ||
@updateTtl={{action "updateTtl"}} | ||
@buttonDisabled={{this.buttonDisabled}} | ||
@errors={{this.errors}} | ||
@data={{this.data}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data down! ⬇️
@@ -49,7 +43,6 @@ export default class ToolWrap extends Component { | |||
codemirrorUpdated(val, codemirror) { | |||
codemirror.performLint(); | |||
const hasErrors = codemirror?.state.lint.marked?.length > 0; | |||
this.data = val; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead, pass to parent callback this.args.codemirrorUpdated
which handles setting data
as the codemirror value
import codemirror from 'vault/tests/helpers/codemirror'; | ||
import { TOOLS_SELECTORS as TS } from 'vault/tests/helpers/tools-selectors'; | ||
|
||
module('Integration | Component | tools/tool-wrap', function (hooks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow-on when glimmerizing is I'm planning to move all of the tool-
components into a tools/
directory
@@ -0,0 +1,3 @@ | |||
```release-note:improvement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I marked this as improvement because I believe the json editor retaining the data is a regression from when we replaced partials with glimmerized child components here which made the component track the data
whereas previously it was cleared and restored to all default values on back.
vault/ui/app/components/tool-actions-form.js
Lines 50 to 55 in 475f324
reset() { | |
if (this.isDestroyed || this.isDestroying) { | |
return; | |
} | |
setProperties(this, DEFAULTS); | |
}, |
However, based on the customer feedback repeatedly wrapping the same data is the desired behavior.
Build Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, no notes. nice job tackling this bugfix and some cleanup at the same time!
@@ -13,8 +13,8 @@ | |||
|
|||
{{#if @unwrap_data}} | |||
<Hds::Tabs as |T|> | |||
<T.Tab data-test-button-data>Data</T.Tab> | |||
<T.Tab data-test-button-details>Wrap Details</T.Tab> | |||
<T.Tab data-test-tab="data">Data</T.Tab> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for all this cleanup 💅
before
After initially wrapping data, clicking "Back" and then "Wrap data" again results in the incorrect data being sent to the backend, so upon "unwrapping" we do not get
{foo:"bar"}
but instead{token:""}
after
Notice that now both generated tokens unwrapped the expected data