Skip to content
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

Form editor #4968

Merged
merged 15 commits into from May 22, 2023
Merged

Form editor #4968

merged 15 commits into from May 22, 2023

Conversation

dennisreimann
Copy link
Member

Adds an editor UI to the custom forms. Closes #4695.

First rough prototype version, to give @dstrukt something to play with.

form-editor

@dennisreimann dennisreimann self-assigned this May 10, 2023
@dennisreimann dennisreimann added Enhancement Improvements to an existing feature UI / UX Front-end issues, for front-end designers labels May 10, 2023
@dstrukt
Copy link
Member

dstrukt commented May 10, 2023

Heeeeeell yeah!

Excited to poke around on this later this evening. Think the latest mock explorations are gonna help move this along quickly!

@dennisreimann
Copy link
Member Author

dennisreimann commented May 12, 2023

The basics are working now. Opening it up for review, though I might do some more polishing.

grafik

Still missing: Editing select options and fieldset fields.

@dennisreimann dennisreimann marked this pull request as ready for review May 12, 2023 14:40
@dennisreimann
Copy link
Member Author

I'd say this is done now, feel free to review :)

@dstrukt
Copy link
Member

dstrukt commented May 14, 2023

Overall, this is fantastic, and very close to the mocks / discussion in the design call.

I made a few text, label, and minor changes, and have a few more suggestions & questions:

Edit Requests:

  • Think we should try to consolidate the Code / Editor into a tabbed section like so (our 24pt font) .. slight deviation from our typical design system tabs, but think it works well here, thoughts?

  • The "templates" section could be a little more bold and larger (16pt font)

  • Constant or Fixed .. constant isn't as clear as it could be imo? Maybe there's a better label than Fixed or Constant...

  • I sometimes mis-clicked on the trash icon, and could definitely see myself doing it on mobile, so propose we move the drag to the left side of the form element, and keep the trash on the right. If we go this route, we could either do: top aligned, center aligned on the row, or center aligned based on the input field, e.g:

Screen Shot 2023-05-13 at 7 43 48 PM Screen Shot 2023-05-13 at 7 43 54 PM

Questions:

  • Is there a notion of "placeholder text" and should there be? Thoughts?

Playing around in the editor - assuming we eventually drop the tabs - think we're reaching that point with these more in-depth editor pages. We could then fix the top section instead of the tabs.
Screen Shot 2023-05-13 at 7 57 15 PM
Looks pretty good!

Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

@dstrukt dstrukt self-assigned this May 14, 2023
@dennisreimann
Copy link
Member Author

dennisreimann commented May 14, 2023

I sometimes mis-clicked on the trash icon, and could definitely see myself doing it on mobile, so propose we move the drag to the left side of the form element

Can you do a mock of this with fieldsets and nested fields? The reason I put both controls on the right and also top-aligned them was that imho this worked better with this nested case.

@dstrukt
Copy link
Member

dstrukt commented May 14, 2023

Still exploring, but wanted to drop progress / ideas. The add form element for each fieldset would obv. sit at the bottom of each section. Tagged in Figma as well.

This may be more difficult / trouble than it's worth to implement, but do think it looks pretty and is quickly parse-able, and if so, we can ship with what we have sans this change!

Screen Shot 2023-05-14 at 12 57 08 AM

Edit, okay added the `+ More'

Screen Shot 2023-05-14 at 1 05 14 AM

@dennisreimann
Copy link
Member Author

Is there a notion of "placeholder text" and should there be? Thoughts?

IMho we don't need it as we already have the label and helper text.

UI updated:

grafik

@dstrukt
Copy link
Member

dstrukt commented May 15, 2023

IMho we don't need it as we already have the label and helper text.

Sounds good .. just wanted to confirm / get a second opinion!

Beautiful, will review this later today as well .. only thing I'm debating is if I miss the borders from your original, but will poke around, think we're really close here!

@dennisreimann
Copy link
Member Author

It felt a bit inconsistent to habe the borders on the root level, but not when nested. So I opted to remove them altogether and I think it works.

However, if you want them back, we can re-add them – but for clarity I'd add them regardless of the nesting then.

@dstrukt
Copy link
Member

dstrukt commented May 15, 2023

Still thinking on:

  • borders
  • bg-tile for the whole form list

Otherwise, this is fucking awesome - fantastic work!

@pavlenex
Copy link
Contributor

pavlenex commented May 16, 2023

Just one note, while tetsing this, I was a bit overwhelmed with what to call to action actually is here. Code seemed like it's the main cta.
There's a lot going on here, can we do anything to make experience more focused?
Screenshot 2023-05-16 135433

@dennisreimann
Copy link
Member Author

Good point, @pavlenex and I made some more improvements:

  • Adjust tab active color (was messed up in light theme) as well as font size and weight to make the tabs less prominent
  • Add first field automatically if there is none
  • Hide drag handles if there is only one element in the list

grafik

@dstrukt
Copy link
Member

dstrukt commented May 16, 2023

Great catches @pavlenex and ACK to all of the "default" state improvements @dennisreimann!

@NicolasDorier
Copy link
Member

need to add test to CheckJsContent for the new lib

@dstrukt dstrukt self-requested a review May 19, 2023 02:28
@dennisreimann
Copy link
Member Author

need to add test to CheckJsContent for the new lib

@NicolasDorier I added it for sortable.min.js, but had to adapt the thin Vue wrapper, because it wasn't compatible with Vue 2. (Details). Alternative wrapper libraries are quite large and as we only need to directive anyways, I decided it would be best to directly edit the file.

@NicolasDorier NicolasDorier self-requested a review May 20, 2023 14:29
@NicolasDorier
Copy link
Member

seems good to me @Kukks want review?

@Kukks Kukks merged commit 44aaf7a into btcpayserver:master May 22, 2023
4 checks passed
@dennisreimann dennisreimann deleted the form-editor branch May 22, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improvements to an existing feature UI / UX Front-end issues, for front-end designers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add UI to Custom Forms
5 participants