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

refactor: Improve Form Builder UX #22968

Merged
merged 39 commits into from Nov 1, 2023

Conversation

shariquerik
Copy link
Member

@shariquerik shariquerik commented Oct 30, 2023

  • Added a dialog to create a new doctype
  • feat: Added on_tab_change event to control tab change logic from doctype's js file
  • Removed unused elements when the "Form" tab is active.
  • Changed form builder layout and moved the Sidebar on the right also removed the double scroll
  • Remove drag to add fields and only keep properties on the right sidebar
  • Add a better way to add fields on the form
  • The field types dropdown is getting clipped (Handled using Teleport)
  • Fix failing UI test
Screen.Recording.2023-10-31.at.2.25.55.AM.mov

@shariquerik shariquerik requested a review from a team as a code owner October 30, 2023 08:52
@shariquerik shariquerik requested review from surajshetty3416 and removed request for a team October 30, 2023 08:52
@shariquerik shariquerik marked this pull request as draft October 30, 2023 08:53
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #22968 (aa0e72d) into develop (d92f74f) will decrease coverage by 0.23%.
Report is 42 commits behind head on develop.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop   #22968      +/-   ##
===========================================
- Coverage    62.19%   61.97%   -0.23%     
===========================================
  Files          770      770              
  Lines        73806    74725     +919     
  Branches      6364     6364              
===========================================
+ Hits         45904    46308     +404     
- Misses       24279    24794     +515     
  Partials      3623     3623              
Flag Coverage Δ
server 70.75% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@shariquerik shariquerik marked this pull request as ready for review October 30, 2023 19:08
@surajshetty3416 surajshetty3416 changed the title refactor: Form Builder UI in DocType form refactor: Improve Form Builder UX Oct 31, 2023
Copy link
Member

@surajshetty3416 surajshetty3416 left a comment

Choose a reason for hiding this comment

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

Nice improvements 👍🏼


Few things to fix

  • Focus on label on creating new field
  • Merge "Details" + "Settings" tabs to "Settings"
  • Add keyboard shortcut for adding (ctrl+shift+n) & selecting fields(arrow keys)
  • Avoid magical delete (add a field in tab 2 and press enter)
  • Delete/Backspace to delete tab/section/column/field if selected

@bosue
Copy link
Contributor

bosue commented Oct 31, 2023

Nice, what a huge improvement!

The message “This site is in developer mode…” should probably appear in all tabs while disappearing if dismissed once.

In the Quick Entry form, the “Custom” checkbox could probably use a description, particularly in contrast to the “Custom” Module. But this could be a followup as well.

@ankush
Copy link
Member

ankush commented Oct 31, 2023

Looking neat 👌

Few observations. Some unrelated to this change, just being pedantic because most used page for developers 😁

  • Bug: Backspace while editing some field property is deleting the field from form. IMO delete is better keybind for this and also shouldn't propagate if it's sent to some other input control.

  • Quick entry is really nice addition! It should have few more fields (because they are kinda necessary on first save):

    • Is virtual (this can't be done after first save, very annoying to update boilerplate)
    • Is tree - for discoverability

image

  • DocField needs one round of cleanup (again)
    • Length should be integer type (or at least displayed as such). This isn't that important so can be moved in some later section.
    • bold can be removed or moved to much later section. Kinda useless.
    • options description can be dynamic based on type.

IMO creation of sections and columns is still kind of hard to understand for newbies 😅 maybe do some blind test with users who haven't used it yet. 😬 (or check posthog session recordings, if any.)

@ankush
Copy link
Member

ankush commented Oct 31, 2023

In the Quick Entry form, the “Custom” checkbox could probably use a description

Custom ? -> Custom DocType

@shariquerik
Copy link
Member Author

shariquerik commented Oct 31, 2023

Bug: Backspace while editing some field property is deleting the field from form. IMO delete is better keybind for this and also shouldn't propagate if it's sent to some other input control.

I knew about this and I was working on it. Now fixed👍🏻

DocField needs one round of cleanup (again)

Yeah, let's not do this in this PR. I will probably ask Ritvik to work on the Properties part. It needs sections and other cleanups.

IMO creation of sections and columns is still kind of hard to understand for newbies 😅 maybe do some blind test with users who haven't used it yet. 😬 (or check posthog session recordings, if any.)

If you see, sections/columns/fields have a + button in the action area and all do the same thing add a section/column/field.
Yes, for sections and columns, we have to click on it and then the action button appears showing it always also doesn't make sense and will look weird.
I think people will learn by using it 🙃. Or maybe we can get some feedback from Timeless.

(or check posthog session recordings, if any.)

let me know how and from where to access this. Will take a look

@shariquerik
Copy link
Member Author

shariquerik commented Oct 31, 2023

Custom ? -> Custom DocType

It is consistent with others, and it is also self-explanatory I don't think the description is needed 🤨

@bosue
Copy link
Contributor

bosue commented Oct 31, 2023

Custom ? -> Custom DocType

It is consistent with others, and it is also self-explanatory I don't think the description is needed 🤨

Consistent with… others? Mhm…

And what happens to make Custom DocTypes special?💁🏻‍♂️

No, really. For a seasoned Frappe developer this may be obvious, but for newcomers it isn't. Not even for newcomers in developer mode.

Especially given how lacking documentation still is, questions like:

  • Aren't all DocTypes created by myself "Custom DocTypes"?
  • Are DocTypes created via the UI "Custom DocTypes"?
  • Are DocTypes in the "Custom" module "Custom DocTypes"?

aren't stupid questions. Neither is mine... 🤷🏻‍♂️

Let's defer it to a followup though. This shouldn't distract from the huge improvements of your undertaking.

@bosue
Copy link
Contributor

bosue commented Oct 31, 2023

There‘s no longer the traditional field list below the form builder, so the scrolling limitations on touchscreens will be even more consequential than it used to be: it‘s simply no longer possible to configure fields below the viewport via the UI.

RPReplay_Final1698791403.mov

The form shouldn‘t be limited to an arbitrary viewport. All other Frappe pages work okay on touchscreens, so should this one…

@shariquerik
Copy link
Member Author

Consistent with… others?

Other settings label e.g Is Submittable, Is Tree. Yeah, I agree they have descriptions.

No, really. For a seasoned Frappe developer this may be obvious, but for newcomers it isn't. Not even for newcomers in developer mode.

I agree, I think we can use the documentation link feature to redirect to the docs

Screen.Recording.2023-11-01.at.1.36.30.PM.mov

@shariquerik
Copy link
Member Author

shariquerik commented Nov 1, 2023

There‘s no longer the traditional field list below the form builder, so the scrolling limitations on touchscreens will be even more consequential than it used to be: it‘s simply no longer possible to configure fields below the viewport via the UI.

The form shouldn‘t be limited to an arbitrary viewport. All other Frappe pages work okay on touchscreens, so should this one…

I will try to figure out a solution for touchscreens

I am trying to remove the double scroll which is kind of annoying while working on form builder.

Edit: Fixed. To move fields long press (200ms) and drag

@shariquerik shariquerik closed this Nov 1, 2023
@shariquerik shariquerik reopened this Nov 1, 2023
@szufisher
Copy link
Contributor

please do not hide fields table, because it is handy to create many fields via excel copy paste via fields table!

@shariquerik shariquerik added the backport version-15-hotfix Backport the PR to v15 label Nov 1, 2023
@shariquerik shariquerik merged commit 45236a9 into frappe:develop Nov 1, 2023
21 of 25 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-15-hotfix Backport the PR to v15
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants