UX Design - Template Step - validation and error display

[dongniwang]

This is a...

[x] Feature request
[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Documentation issue or request

The problem

• It's not clear to users that validation is happening via text editor when confirguring a template step.
• There's no information about validation errors.

Expected behavior

• Need to tell users that we are conducting live validation through the template step text editor. Help text informing users about validation.
• Also, need to provide error information in a more user-friendly (human-readable) way.

Screenshot

https://redhat.invisionapp.com/share/VQ8UZ7PCE#/318752828_Template_Step_V3-1_9-6_3

@michael-coker @phantomjinx
cc: @syndesisio/uxd

[dongniwang]

@phantomjinx - is this something we hope to get in for 7.2? I'm asking because it's not required from the user story #3047.

Do you have screenshots of what these validation errors may look like? How are they different from just the validation happening in the text editor?

[phantomjinx]

@dongniwang
Screenshot of error as it appears in console log. It does actually do this now so you should be able to view it yourself. This is the full stacktrace while the error message is 'Error: Unclosed tag at 81' (note. syndesis code prefixes the error with 'Failed to parse template')

To your broader question, we should be able to get this in before the end of the sprint. A simple error message that appears underneath the editor box, updates on each keypress and disappears if parsing is fine should not be a problem, I think.

There is a slight wrinkle that may need looking at. A further parsing check is required; to explain:

• The current parsing of the template defers to the mustache library parsing function;
• The mustache library parses the library but is deficient in the following situation:
  • Template (fully parseable) -> At {{body.time}}, {{body.name}} submitted the following
  • Template (also fully parseable) -> At {{body.time}}, {{body.name}} submitted the following}}
  • Basically, the parsing simply looks for opening and closing braces and includes EVERYTHING between then, including other braces. This is crazy since it is obviously a typing error.

Therefore, an extra parsing function needs to be included to enhance mustache's parsing.

@gaughan @michael-coker

[phantomjinx]

Design decisions from follow-up meeting:

• Page 3
  • Retain the centre placeholder label but use js/css to remove it upon user editor focus (as opposed to using CodeMirror's own placeholder function);
• Page 4
  • Use CodeMirror's own DnD functionality for adding text from file. However, enhance it to retain an icon of the file upon drop;
  • Retain the 'Drop file here to upload' message. Maybe gets displayed while user drags? (@michael-coker can you just clarify that?);
• Page 5
  • Uploading of dragging in a file does highlight the dropped-in content but not an issue and could be useful;
• Page 6 & 7
  • Error validation to occur after every keystroke as it is now;
  • Error validation 'detail' to be closed by default, only showing 'Validation errors (x)' message only.
  • If there are errors then Save is disabled, ie. syntax must be valid to continue;
• Page 9
  • Both DnD and upload-from-file overwrite, ie. the user performs either then the current content of the editor will be replaced.

@dongniwang @michael-coker - review please.

[mcoker]

Use CodeMirror's own DnD functionality for adding text from file. However, enhance it to retain an icon of the file upon drop;

If that's possible, sounds great. But I would be fine with the browser default behaviors for dragging a file. That would also match our current DnD UI in the integration importer and API client connector upload.

Retain the 'Drop file here to upload' message. Maybe gets displayed while user drags? (@michael-coker can you just clarify that?);

Yep, I can handle that by manipulating a class on the main element on the dragover, dragleave, and drop events.

[dongniwang]

@phantomjinx @michael-coker - looks good to me, thanks so much for capturing the decisions! I'll upload the design file to the UX design tracker as well.

[mcoker]

@phantomjinx fyi, came across this, not sure if you've seen it - https://codemirror.net/demo/lint.html

that looks like a good way to show code errors.

[phantomjinx]

On 02/10/18 22:09, Michael Coker wrote: @phantomjinx <https://github.com/phantomjinx> fyi, came across this, not sure if you've seen it - https://codemirror.net/demo/lint.html that looks like a good way to show code errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3662 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABjvhAVLMyakecZaYxVde6ExJ_iv74KHks5ug9YNgaJpZM4W3UPx>.

Hi Michael, Thanks, looked into it, run with it and will push a commit for your review, by hopefully, end of (my) day. Dongni: Are you ok with varying the design to include this? Cheers PGR

…

-- Paul Richardson * p.g.richardson@phantomjinx.co.uk * Ty-Carreg Barn, Penyrheol, Pontypool, Torfaen. NP4 5XZ * (home) 01495 762483 * (mob) 07980 869490 "There is no operating system on the planet that can withstand a determined assault from a clueless user." "I know exactly who reads the papers ... * The Daily Mirror is read by people who think they run the country. * The Guardian is read by people who think they ought to run the country. * The Times is read by people who do actually run the country. * The Daily Mail is read by the wives of the people who run the country. * The Financial Times is read by the people who own the country. * The Morning Star is read by the people who think the country ought to be run by another country. * The Daily Telegraph is read by the people who think it is." Jim Hacker, Yes Minister

[dongniwang]

@phantomjinx - yes! That's totally fine with me. Thank you!

[phantomjinx]

@michael-coker

Ok.... here is the 2 commits for the new and improved editor ... my-branch

It should be good to test but let me know any problems, including if you need a PR instead?

cc @dongniwang @gaughan

[mcoker]

@phantomjinx great, thanks! I'll work from your branch.

[mcoker]

@phantomjinx the validation stuff looks great!! @dongniwang now that we have that inline validation in place in the editor itself, do we need to show the validation errors below the template editor? When you hover over the validation errors in the template editor, you get the details about the validation error.

[phantomjinx]

@michael-coker thanks. So I left the validation-under-editor in just because its per the design (and took a couple of hours last night to get right!). However, it is now redundant given mostly everything it displays in now inline.

I say mostly because the MustacheMode class does not delegate to the Mustache library's own parse function, whereas the onChange() function of templateComponent.ts does. The purpose of calling this is to get Mustache to break up the text into symbols and text which then form the specification of the OutputDataShape. However, it does also spit out an errors if the text doesn't parse. The downside of this error is that it does not do it by line/column but by character number so doesn't fit with the inline error messages.

• Do we need this extra parse error message? Probably not.
• If we really do, can we convert character index to line/column? Probably but its another thing for the validation to do and we kinds already have that information.
• Does delegating to Mustache.parse serve any other purpose during valdating? The only benefit I can think of is at some stage the mustache library improves its parsing and produces more useful error messages that might be beneficial.

Therefore, probably put a pin in this one and revisit maybe at later stage.

[dongniwang]

Yeah, I think the inline validation looks pretty awesome! Thank you @phantomjinx and @michael-coker for working on this!

I agreed that the error message below the editor is not providing additional useful and helpful information and we can remove that for the template step. However, I think it's still a good functionality to keep in our back pocket and it could be very useful if we have other types of error messages we want to display or to complement the editor in the future. Thank you so much @phantomjinx for spending hours on this and I really appreciate you took the effort to match UI implementation with the UX design!

[mcoker]

Hey @phantomjinx, I made some minor updates to the UI and got the placeholder/hover overlay text working except that the dragenter event only fires when you hover over the border of the codemirror editor. As soon as the mouse enters, the dragleave event is fired and the hover overlay text disappears. Looks like it's possibly due to hovering a child element in the codemirror element. I added (and commented out) some SCSS that sets pointer-events: none; on all of the codemirror element's children once you've moused over the element, and if you uncomment that, you can see the hover state/overlay works as intended, but the file drop doesn't work. That's just to demonstrate the issue.

Do you have any ideas? Those events seem to work fine in this example - http://jsfiddle.net/3pvvLote/69/

Here is my branch

Also a small thing, but I noticed you can drop non .tmpl files in the editor. Dropping a .txt file will upload the file's contents to the editor.

And I removed the validation output from the HTML/CSS templates but left the typescript in there for you to remove in case it's used in a way I don't understand.

[phantomjinx]

Also a small thing, but I noticed you can drop non .tmpl files in the editor. Dropping a .txt file will upload the file's contents to the editor.

So, by default, codemirror will let any file be dropped. This can be limited by included allowDropFileTypes in the configuration. The problem is specifying what the file-types are that can be included since it requires the MIME type of the file rather than the extension. A standard .tmpl text file has no MIME type and therefore is rejected. It is possible to get the $event during the drop and interrogate the DataTransfer object but that could be browser/OS specific and a real can-o-worms.

@michael-coker

[phantomjinx]

@michael-coker
As regards the DnD issue, I am stumped.

I'll go ahead and create a PR just to get the rest of it in as I don't think this the issues are detrimental to the whole.

[mcoker]

@dongniwang @phantomjinx Just wanted to touch base about this and see if we can re-visit the 2 remaining issues from this. Not sure if I should open another issue or want to keep track of these here?

• You can drop any type of file in the codemirror editor.
• The “hover” overlay (drop files here) text doesn’t appear.

My initial thought about being able to drop any kind of file in the codemirror editor is, from what I tested, codemirror allows files with text in them (txt, json, css, html, hbs, etc), but not files that aren't just text (PDF, jpg, etc). What if we changed the "allowed file types" text to match whatever codemirror's restrictions are?

Also, if you try to drop a PDF in the codemirror editor, there is no error message. Can we display one?

And I'm not sure about the hover overlay dragleave event firing unexpectedly. Seems like a bug we can probably work out if we spend some time on it.

[dongniwang]

@michael-coker @phantomjinx - Assuming with the latest work from #3763, we can close this issue now?

[mcoker]

@dongniwang I think so, all of my original issues have been addressed.

[dongniwang]

Awesome, closing now. Will open new issues if needed. Thank you!