You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/08/07 21:56:17 UTC
[GitHub] [druid] vogievetsky opened a new pull request #10256: Web console: improve JSON paste experience
vogievetsky opened a new pull request #10256:
URL: https://github.com/apache/druid/pull/10256
Fix some UX nits where it was not possible to clear the spec box in the `Edit spec` screen because it would always fill it with `{}`. Make the spec submission dialog not default to `{ }` but use placeholder instead.
Just look at that empty input box. Is is mind boggling what technology can achieve today... from heavier than air flight to NOT putting text in a box. Truly remarkable.
This PR has:
- [x] been self-reviewed.
(Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467312311
##########
File path: web-console/src/dialogs/spec-dialog/__snapshots__/spec-dialog.spec.tsx.snap
##########
@@ -1,6 +1,187 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`spec dialog matches snapshot 1`] = `
+exports[`spec dialog matches snapshot no initSpec 1`] = `
Review comment:
Oh, interesting…
Do we bundle this with the released app or is it skipped? I didn't realize it was a test file at first, because it's in the same `src` path that the main code is in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467332609
##########
File path: web-console/src/dialogs/spec-dialog/__snapshots__/spec-dialog.spec.tsx.snap
##########
@@ -1,6 +1,187 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`spec dialog matches snapshot 1`] = `
+exports[`spec dialog matches snapshot no initSpec 1`] = `
Review comment:
TIL
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467314056
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -24,7 +24,9 @@ import AceEditor from 'react-ace';
import './json-input.scss';
function parseHjson(str: string) {
- return str === '' ? null : Hjson.parse(str);
+ // Throwing on empty input is more consistent with how JSON.parse works
Review comment:
When it can not parse the editor itself would keep the partial string value inside of it - so nothing interesting would happen. The issue is when it does parse... in simple terms this was the old problem:
Let say you were deleting text and you have `{` entered. Then you press backspace.
The simplified logic flow would go like this:
- AceEditor: user has changed input to `''`
- JsonInput: `''` is valid JSON it parses to `null`, notify the parent
- DataLoader: user changed the spec to `null` that is not workable as a spec, adjust it to `{}`
- JsonInput: adjusting the input to `"{}"`
- AceEditor: shows `{}` after you pressed backspace on `{`
Now it would go like this:
- AceEditor: user has changed input to `''`
- JsonInput: `''` is not valid JSON because it throws, keep waiting
- AceEditor: shows nothing
Note that `Hjson.parse({})` is `{}` so that is why we need to special case white space only strings. If we were not being that fancy and using boring old `JSON.parse` this would not be an issue as `JSON.parse` does not recognize `''` as valid json.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467310965
##########
File path: web-console/src/dialogs/spec-dialog/spec-dialog.tsx
##########
@@ -65,6 +65,7 @@ export const SpecDialog = React.memo(function SpecDialog(props: SpecDialogProps)
tabSize: 2,
}}
style={{}}
+ placeholder="{ JSON spec... }"
Review comment:
There are two changes in this PR that are thematically similar (when you fix an issue it is better to look for other potential issues in the code). A placeholder is the ghost text that appears in an input that goes away as soon as you start typing.
Before this change this dialog (which is not part of the data loader) would start of with `{\n\n}` as the initial value.
Now it start off blank (but with a placeholder) meaning that you do not need to delete or select over the initial value if you want to paste your own spec.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467331434
##########
File path: web-console/src/dialogs/spec-dialog/__snapshots__/spec-dialog.spec.tsx.snap
##########
@@ -1,6 +1,187 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`spec dialog matches snapshot 1`] = `
+exports[`spec dialog matches snapshot no initSpec 1`] = `
Review comment:
none of the tests (and test aids like this one) are included in the bundle because none of the code depends on any of the test code (if it did then it would not compile because tests run in a different environment `jest` than the prod code `node/dom`).
In general the web console follows a standard where every class (or functional component) lives in a file next to the style and the test
... this has been the case from day one (of the unified console) and is pretty idiomatic of the modern web-dev world.
The tests are separated from the code with file extensions `*.spec.*` vs location
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467311498
##########
File path: web-console/src/dialogs/spec-dialog/__snapshots__/spec-dialog.spec.tsx.snap
##########
@@ -1,6 +1,187 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`spec dialog matches snapshot 1`] = `
+exports[`spec dialog matches snapshot no initSpec 1`] = `
Review comment:
The `.snap` files are snapshots, they are unit tests that virtually render the UI and then take a snapshot of the dom.
The `XXXXXXXXXXXX` is some part of the internals of the AceEditor this is the 3rd party component we use to give the IDE like editor for the JSON. What does it do? No idea, but we will know if it changes!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467307678
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -24,7 +24,9 @@ import AceEditor from 'react-ace';
import './json-input.scss';
function parseHjson(str: string) {
- return str === '' ? null : Hjson.parse(str);
+ // Throwing on empty input is more consistent with how JSON.parse works
Review comment:
What happens when the text in the area can't be parsed?
##########
File path: web-console/src/dialogs/spec-dialog/__snapshots__/spec-dialog.spec.tsx.snap
##########
@@ -1,6 +1,187 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`spec dialog matches snapshot 1`] = `
+exports[`spec dialog matches snapshot no initSpec 1`] = `
Review comment:
What's all this new stuff for and what are the `XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX` about?
##########
File path: web-console/src/dialogs/spec-dialog/spec-dialog.tsx
##########
@@ -65,6 +65,7 @@ export const SpecDialog = React.memo(function SpecDialog(props: SpecDialogProps)
tabSize: 2,
}}
style={{}}
+ placeholder="{ JSON spec... }"
Review comment:
What does `placeholder` do?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm merged pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #10256:
URL: https://github.com/apache/druid/pull/10256
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a change in pull request #10256: Web console: improve JSON paste experience
Posted by GitBox <gi...@apache.org>.
gianm commented on a change in pull request #10256:
URL: https://github.com/apache/druid/pull/10256#discussion_r467332588
##########
File path: web-console/src/components/json-input/json-input.tsx
##########
@@ -24,7 +24,9 @@ import AceEditor from 'react-ace';
import './json-input.scss';
function parseHjson(str: string) {
- return str === '' ? null : Hjson.parse(str);
+ // Throwing on empty input is more consistent with how JSON.parse works
Review comment:
Thanks for the detailed explanation!!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org