Fedict / eid-mw

eID Middleware (main repository)
GNU Lesser General Public License v3.0
198 stars 79 forks source link

Change the "Validate Now" button #203

Closed acoomans closed 3 months ago

acoomans commented 4 months ago

Fixes the "Validate Now" button on the "Certificates" tab.

BEFORE: The button looked disabled

Screenshot 2024-05-03 at 21 01 01

AFTER: the button looks clickable

Screenshot 2024-05-03 at 20 55 09
yoe commented 3 months ago

The "validate now" button is supposed to be disabled until there is something to validate. This means that it should be disabled at startup, until the newstate:state method in AppDelegate.m enables it.

I would prefer a patch which updates the look property there rather than in the .xib

acoomans commented 3 months ago

@yoe This fix is purely a UI design change: it uses a regular button instead of a push button, which is more clear. The fix does NOT change the enable/disable behavior, which is and remains correct: the button is disabled when there's nothing to validate, and is enabled if something can be validated.

See the following videos (note I only click but do not release the button at the end of the video as I don't want to actually validate the certs on this machine, but it shows that the correct button is now used).

Before:

When there is nothing to validate: disabled, cannot click -> ok When there is something to validate: button is a push button, looks like it's disabled although it's not -> this is confusing to the user

https://github.com/Fedict/eid-mw/assets/733352/1e3ec2ff-e1eb-4cf1-b844-4e8e52367cde

Fix:

When there is nothing to validate: disabled, cannot click -> ok (note: behavior is unchanged from before) When there is something to validate: button is a regular button and looks clickable to the user -> not confusing

https://github.com/Fedict/eid-mw/assets/733352/621e5886-ecc4-45de-ac54-f960b9bebbef

yoe commented 3 months ago

Oh, okay. Sorry, misunderstood. That's fine, then.

acoomans commented 3 months ago

No worries. Maybe the initial description of my PR wasn't clear enough. Glad to see this merged in.