Add custom errorLabel to MaterialTextField #1901

ItoroD · 2024-03-23T01:14:53Z

I replaced helpLabel with error label where necessary. Tested validation and it works correctly. This should avoid any bind errors
@HenrikJannsen

Add custom errorLabel to MaterialTextField #1783

HenrikJannsen

NACK
If the help text is used and the validation triggers an error the error is displayed above the help.

The // TODO add custom errorLabel... can be removed.
At onWidthChanged the errorLabel.setPrefWidth(width - 2 * errorLabel.getLayoutX()); is missing

Best to check the reformat checkbox at the commit interface in IntelliJ so that code is correctly formatted (just a missing space where you add the errorLabel)

In preferences the min required reputation use a help field and can be used for testing.

ItoroD · 2024-03-23T10:01:54Z

@HenrikJannsen I have made the required changes. It was best to set the help to invisible when there is an error message and visible when no error. Other option was making help empty but that made the components below move up so that was not a good option.

HenrikJannsen · 2024-03-23T10:19:01Z

@HenrikJannsen I have made the required changes. It was best to set the help to invisible when there is an error message and visible when no error. Other option was making help empty but that made the components below move up so that was not a good option.

Does the error message replace the help text? So if the error message gets cleared the help text is back?

ItoroD · 2024-03-23T10:20:12Z

@HenrikJannsen I have made the required changes. It was best to set the help to invisible when there is an error message and visible when no error. Other option was making help empty but that made the components below move up so that was not a good option.

Does the error message replace the help text? So if the error message gets cleared the help text is back?

Yes. When error is cleared help message is back

ItoroD · 2024-03-23T10:47:50Z

https://recordit.co/pOJNoUPsde

@HenrikJannsen

ItoroD · 2024-03-23T14:55:25Z

@HenrikJannsen

any other thing I need to do here? Is this good for merge?
so I can move to another issue.

HenrikJannsen · 2024-03-24T05:27:42Z

The height is not updated if there is no help text. I guess the update() method calls are missing.

HenrikJannsen

NACK

apps/desktop/desktop/src/main/java/bisq/desktop/components/controls/MaterialTextField.java

ItoroD · 2024-03-24T12:38:38Z

The height is not updated if there is no help text. I guess the update() method calls are missing.

No this is because this is a Vbox component and spacing between them have been set to 10. So it does not care whether we have an error to display or not because the error text is not part of the first Vbox component like help text is (for places we have help)

VBox networkVBox = new VBox(10, difficultyAdjustmentFactor, ignoreDiffAdjustFromSecManagerSwitch);

when i change the spacing:

VBox networkVBox = new VBox(20, difficultyAdjustmentFactor, ignoreDiffAdjustFromSecManagerSwitch);

My suggestion is simple

for places where there is no help text and component is vbox, we give Vbox spaceing of 20 to accommodate for when there is error
for places where there is help text if we use the helpLabel.setManaged(false); helpLabel.setVisible(false); like you suggested, when we show error we will have the same spacing issue as you have pointed out. Because vbox for component with help also have spacing 10.
VBox tradeVBox = new VBox(10, minRequiredReputationScore, offersOnlySwitch, closeMyOfferWhenTaken);

If we use opacity to make help not visible we maintain the correct spacing for error to display (by using helps space)

ItoroD · 2024-03-25T03:19:37Z

@HenrikJannsen updated the code

HenrikJannsen · 2024-03-25T05:13:37Z

I am busy atm. Will review a bit later. Anyone else can review as well.

ItoroD · 2024-03-25T05:42:33Z

I am busy atm. Will review a bit later. Anyone else can review as well.

ok

HenrikJannsen

NACK

This is not the appropriate change to make the component generic. We do not want to adjust the outside container but the height calculation should take care of the potential existence of the error or help field. With the error field getting visible the height should gets updated so that the outside container re-layouts and maintains the intended spacing.

I will not review personally more PR's as my time would be better spent by fixing it myself. Any other contributor though is welcome to review upcoming PRs. Just wanted to give you my feedback so that you can manage expectations.

ItoroD · 2024-03-28T09:32:28Z

Noted @HenrikJannsen

ItoroD · 2024-03-29T16:16:16Z

@HenrikJannsen I know you have been very patient with me and this PR in taking the time to look at it as you have other pressing issues to look at. Only now do I see what you mean by height calculation should take of the error text showing. Should have seen this earlier (my bad)

last commit

uses setManaged and setVisible instead of setopacity
added height calculation for error text label.
reverted changed height of vbox to what it was before

if you have not fixed this already and theres a 1% chance of taking a look at this, it will be great as I am now getting familiar with the code base.

axpoems

NACK

The following comment is not addressed with the latest changes. Help text is not getting back:

Does the error message replace the help text? So if the error message gets cleared the help text is back?

apps/desktop/desktop/src/main/java/bisq/desktop/components/controls/MaterialTextField.java

ItoroD · 2024-03-30T08:27:34Z

NACK

The following comment is not addressed with the latest changes. Help text is not getting back:

Does the error message replace the help text? So if the error message gets cleared the help text is back?

Hi @axpoems help text does come back for places that had help text. Not all fields have help text except if there is another part of the app I am not seeing this

axpoems · 2024-03-30T08:34:59Z

Once I clear I would expect to see the help text.

ItoroD · 2024-03-30T08:37:10Z

@axpoems This is what I have. And I have pushed this

see video:

https://recordit.co/r5Qjsv3THF

ItoroD · 2024-03-30T08:38:47Z

the field cannot be left empty as empty is also an error. it has to be a number placed in there. At least that was how it worked before

ItoroD · 2024-03-30T08:44:06Z

Once I clear I would expect to see the help text.

So until there is a number placed in the field error will not go away. So even if it empty error does not go "Must be Number"

ItoroD · 2024-03-30T09:06:30Z

I have enabled reformat code now.

djing-chan · 2024-03-31T09:05:30Z

@ItoroD Is the PR ready for review?
e.g.

Tested with all the use cases (with and without help text)
Changing the height when the error text comes in the no help text use case and removes the error text once the validator has no error

E.g. In case the PR is not ready for review (e.g. under dev) please convert it to a draft PR.

ItoroD · 2024-03-31T09:45:21Z

@djing-chan yes it is ready for review.

I was hoping anyone or @axpoems will retest has he made a mistake in testing. Appropriate value should be put in field before error clears for help text to come back.

Height changes now when error shows

djing-chan · 2024-04-01T06:23:01Z

@ItoroD
It is not adjusting the height:

When the error message is not visible the overall height of the component should be adopted to the height needed.

ItoroD · 2024-04-01T11:30:15Z

@djing-chan
revert resize was missing after resize

short.mp4

djing-chan · 2024-04-02T04:39:48Z

@djing-chan revert resize was missing after resize

short.mp4

Looks ok now from the video.
Can you squash all your commits and rebase to main?
I will give it a final review afterwards.

ItoroD · 2024-04-03T12:17:57Z

@djing-chan

rebase done.

djing-chan · 2024-04-03T17:54:31Z

Squashing all commits on top of the rebase would be good. You can select your commits in IntelliJ in the Github commit history and squash them. Then run a rebase on main.

resize of component when error is shown. Also bring back helptext when error disappears changed helpLabel code to errorLabel code for error validation made helplabel invisible when error label shows removed commented code. Setting errorlabel to help is unecessary changed spacing of vbox to accomodate error message removed setopacity and used setmanage instead. Made height to be calculated automatically for error text removed unecessary commented code enabled reformat code added code to make height go back to original position after resize Check that error style does not exist before you set error style

ItoroD · 2024-04-04T13:17:50Z

this better? @djing-chan

HenrikJannsen

ACK

Rebase was still not ok. As this PR takes too much effort with reviews I made an own PR here:
#1992

And a follow up PR with further improvements at #1993

You still can make a compensation request for your work included in that PR.

Add custom errorLabel to MaterialTextField (rebase of #1901)

HenrikJannsen requested changes Mar 23, 2024

View reviewed changes

alvasw changed the title ~~Add custom errorLabel to MaterialTextField and not reuse helpLabel as it would cause an exception when binding at the helpLabel is used #1783~~ Add custom errorLabel to MaterialTextField Mar 23, 2024

HenrikJannsen requested changes Mar 24, 2024

View reviewed changes

apps/desktop/desktop/src/main/java/bisq/desktop/components/controls/MaterialTextField.java Outdated Show resolved Hide resolved

HenrikJannsen requested changes Mar 28, 2024

View reviewed changes

axpoems requested changes Mar 30, 2024

View reviewed changes

axpoems reviewed Mar 30, 2024

View reviewed changes

apps/desktop/desktop/src/main/java/bisq/desktop/components/controls/MaterialTextField.java Outdated Show resolved Hide resolved

alvasw requested a review from HenrikJannsen March 30, 2024 09:35

ItoroD requested a review from axpoems March 30, 2024 10:07

ItoroD force-pushed the Add-custom-errorLabel-to-MaterialTextField-and-not-reuse-helpLabel-as-it-would-cause-an-exception-when-binding-at-the-helpLabel-is-used-#1783 branch from 198508b to e519935 Compare April 4, 2024 13:01

HenrikJannsen approved these changes Apr 4, 2024

View reviewed changes

This was referenced Apr 4, 2024

Improve material text field validation #1991

Closed

Add custom errorLabel to MaterialTextField (rebase of #1901) #1992

Merged

HenrikJannsen closed this Apr 4, 2024

djing-chan added a commit that referenced this pull request Apr 5, 2024

Merge pull request #1992 from HenrikJannsen/rebased-1901

c27083f

Add custom errorLabel to MaterialTextField (rebase of #1901)

axpoems mentioned this pull request May 2, 2024

For Cycle 58 bisq-network/compensation#1544

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom errorLabel to MaterialTextField #1901

Add custom errorLabel to MaterialTextField #1901

ItoroD commented Mar 23, 2024 •

edited

Loading

HenrikJannsen left a comment •

edited

Loading

ItoroD commented Mar 23, 2024

HenrikJannsen commented Mar 23, 2024

ItoroD commented Mar 23, 2024

ItoroD commented Mar 23, 2024

ItoroD commented Mar 23, 2024 •

edited

Loading

HenrikJannsen commented Mar 24, 2024

HenrikJannsen left a comment

ItoroD commented Mar 24, 2024 •

edited

Loading

ItoroD commented Mar 25, 2024

HenrikJannsen commented Mar 25, 2024 •

edited

Loading

ItoroD commented Mar 25, 2024

HenrikJannsen left a comment

ItoroD commented Mar 28, 2024

ItoroD commented Mar 29, 2024 •

edited

Loading

axpoems left a comment

ItoroD commented Mar 30, 2024

axpoems commented Mar 30, 2024

ItoroD commented Mar 30, 2024 •

edited

Loading

ItoroD commented Mar 30, 2024 •

edited

Loading

ItoroD commented Mar 30, 2024

ItoroD commented Mar 30, 2024

djing-chan commented Mar 31, 2024

ItoroD commented Mar 31, 2024 •

edited

Loading

djing-chan commented Apr 1, 2024

ItoroD commented Apr 1, 2024

djing-chan commented Apr 2, 2024

ItoroD commented Apr 3, 2024

djing-chan commented Apr 3, 2024

ItoroD commented Apr 4, 2024

HenrikJannsen left a comment •

edited

Loading

Add custom errorLabel to MaterialTextField #1901

Add custom errorLabel to MaterialTextField #1901

Conversation

ItoroD commented Mar 23, 2024 • edited Loading

HenrikJannsen left a comment • edited Loading

Choose a reason for hiding this comment

ItoroD commented Mar 23, 2024

HenrikJannsen commented Mar 23, 2024

ItoroD commented Mar 23, 2024

ItoroD commented Mar 23, 2024

ItoroD commented Mar 23, 2024 • edited Loading

HenrikJannsen commented Mar 24, 2024

HenrikJannsen left a comment

Choose a reason for hiding this comment

ItoroD commented Mar 24, 2024 • edited Loading

ItoroD commented Mar 25, 2024

HenrikJannsen commented Mar 25, 2024 • edited Loading

ItoroD commented Mar 25, 2024

HenrikJannsen left a comment

Choose a reason for hiding this comment

ItoroD commented Mar 28, 2024

ItoroD commented Mar 29, 2024 • edited Loading

axpoems left a comment

Choose a reason for hiding this comment

ItoroD commented Mar 30, 2024

axpoems commented Mar 30, 2024

ItoroD commented Mar 30, 2024 • edited Loading

ItoroD commented Mar 30, 2024 • edited Loading

ItoroD commented Mar 30, 2024

ItoroD commented Mar 30, 2024

djing-chan commented Mar 31, 2024

ItoroD commented Mar 31, 2024 • edited Loading

djing-chan commented Apr 1, 2024

ItoroD commented Apr 1, 2024

djing-chan commented Apr 2, 2024

ItoroD commented Apr 3, 2024

djing-chan commented Apr 3, 2024

ItoroD commented Apr 4, 2024

HenrikJannsen left a comment • edited Loading

Choose a reason for hiding this comment

ItoroD commented Mar 23, 2024 •

edited

Loading

HenrikJannsen left a comment •

edited

Loading

ItoroD commented Mar 23, 2024 •

edited

Loading

ItoroD commented Mar 24, 2024 •

edited

Loading

HenrikJannsen commented Mar 25, 2024 •

edited

Loading

ItoroD commented Mar 29, 2024 •

edited

Loading

ItoroD commented Mar 30, 2024 •

edited

Loading

ItoroD commented Mar 30, 2024 •

edited

Loading

ItoroD commented Mar 31, 2024 •

edited

Loading

HenrikJannsen left a comment •

edited

Loading