Fix #85 - Invalid icon & validation improvements - #121
Open
tennox wants to merge 12 commits into
Open
Conversation
added 3 commits
November 24, 2019 17:18
File.new_for_path("") will just resolve to the working directory, and as that exists, it tries to load the working directory as icon.
I also changed path detection from `.contains ("/")` to `.has_prefix ("/")` as I think we always have absolute paths. And I'm not sure if theme icon names could contain a '/'.
In some edge cases, the UI doesn't show if fields are invalid - so the user doesn't know why the Save button is greyed out. That's the UX problem of e.g. artemanufrij#85 I refactored so that Form field error status is updated in .validate (now called 'updateFormStatus'), and the change listeners only set the validity boolean. Also update the icon back to default if the name field is cleared. I also added validation feedback for the image button (although it feels weird to set class 'destructive-action', I found it to be the easiest :P)
ryonakano
reviewed
Dec 5, 2019
ryonakano
left a comment
ryonakano
left a comment
There was a problem hiding this comment.
It might be nitpicking, but left some coding style comments.
Comment on lines
+568
to
+574
|
|
||
| public void set_class(Gtk.Widget widget, string class_name, bool flag) { | ||
| if (flag) | ||
| widget.get_style_context().add_class(class_name); | ||
| else | ||
| widget.get_style_context().remove_class(class_name); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Why don't you include this method in the Editor class?
Author
There was a problem hiding this comment.
I thought static was better - then we could move it to a utils file in case it's needed somewhere else 🤷♂️
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
Co-Authored-By: Ryo Nakano <26003928+ryonakano@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Well - first, the 'edit' view showed "create new web app".
Then the bug:
When no icon is found / selected, the icon_name is empty, but later the on_accept code checks
File.new_for_path ("").query_exist ()- which will just resolve to the working directory, and as that exists, it tries to load the working directory as icon:I fixed that by wrapping with
icon.has_prefix("/")And I also worked on the UX for invalid fields (tennox@0d82f02)
In some cases, the UI didn't show if fields are invalid - so the user doesn't know why the Save button is greyed out.
That's the UX problem of e.g. #85 (you don't see the icon is considered 'invalid')
I refactored so that Form field error status is updated in
.validate(now calledupdateFormStatus), and the change listeners only set the validity boolean.Other things I fixed along the way
update the icon back to default if the name field is cleared.
I also added validation feedback for the image button (although it feels weird to set class 'destructive-action', I found it to be the easiest :P)
I also changed path detection from
.contains ("/")to.has_prefix ("/")as I think we always have absolute paths. And I'm not sure if theme icon names could contain a '/'.