Update UX on :style usage #1141
Conversation
| const statements = shouldEnableMultiStatementMode(store.getState()) | ||
|
|
||
| // Semicolons in :style grass break parsing of multiline statements from codemirror. | ||
| const useMultiStatement = |
There was a problem hiding this comment.
The best way I've found to solve the issue of extractStatementsFromString from cypher-codemirror can't parse grass is to avoid using it for style commands. We can't run style and some other command so it makes sense not to use multi-statement anyway. I'm not 100% happy with the magic string of ":style" and it's special handling to other commands though :/
There was a problem hiding this comment.
Im not sure how 'special' situations are handled in Browser or if at all. The only comment I would have is whether a @todo comment here makes sense too or adding :style to some variable that holds non multi statement commands? May not be relevant, Im not sure
There was a problem hiding this comment.
When the allow multi statement setting is turned on, some commands (including :style) can't be part of a multistatement but should still work on their own. The problem is that :style is parsed in such a way that browser thinks it's multiple statments, since cypher-code mirror util we have can't parse grass to split the commands properly. So i don't think the variable would be helping and I'm not sure what to even write in a todo comment.. I'll write it down and discuss with hane/emil when they get back
| @@ -709,10 +709,12 @@ const availableCommands = [ | |||
| name: 'style', | |||
| match: cmd => /^style(\s|$)/.test(cmd), | |||
| exec(action, cmdchar, put, store) { | |||
| const match = action.cmd.match(/:style\s*(\S.*)$/) | |||
There was a problem hiding this comment.
This regex didn't allow for style with newlines
| } | ||
|
|
||
| if (param === 'reset') { | ||
| updateGrassAndShow(null) |
There was a problem hiding this comment.
I've changed it to show a new pane on :style reset and :style node {color: red} to make it more obvious that something actually happens. If a user wants to see how their graph is changed they'd need to pin it for it not to scroll away though. It's not perfect this way but we want to rework how grass-editing works so it good for now
| frames.add({ | ||
| useDb: getUseDb(store.getState()), | ||
| ...action, | ||
| error: createErrorObject(InvalidGrassError, message), |
There was a problem hiding this comment.
Add a custom error screen for grass instead of sending "unknown error"
OskarDamkjaer
requested review from
pe4cey and
jk05
and removed request for
nglgzz
|
|
||
| if ( | ||
| isValidURL(param) && | ||
| param.includes('.') /* isValid url considers words like rest an url*/ |
There was a problem hiding this comment.
is this comment suggesting isValid url considers words like "rest" a url?
There was a problem hiding this comment.
yes, It would be clearer if i added quotes perhaps? Even better would be to update the isValidURL to use a better regex though. It was used in quite a lot of places so I wasn't comfortable doing the change when I wrote this. I could look into it now though
Fixes this problem where semicolons break style when using multiline.
When the :style command is run to create new styles, the updated styles are shown. There's some behaviour changes, they are outlined as comments.