-
Notifications
You must be signed in to change notification settings - Fork 103
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
dlgStringHandling #5848
dlgStringHandling #5848
Conversation
instat/dlgStringHandling.vb
Outdated
@@ -289,7 +289,7 @@ Public Class dlgStringHandling | |||
Me.Size = New Size(iFullWidth, Me.Height) | |||
Else | |||
grpRegex.Visible = False | |||
Me.Size = New Size(iFullWidth / 1.57, Me.Height) | |||
Me.Size = New Size(536, Me.Height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned about this kind of change. Calculation of width and height by a factor e.g iFullWidth / 1.57
is meant to enable resizing of dialogs to adapt well in computers with different resolutions. I think we should stick with that rather than fixing height e.g 536
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, that makes a lot of sense. I will fix that now - thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted it
Please add the previous and the current designer look. |
The designer looks good to me now. However, I noted that the code could be refactored better on the linking bits which are done manually. Since the regex keyboard is not working at the moment I think we can do it once it is enabled. @rdstern @lloyddewit can you review now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now looks much neater and I am happy to approve. So, subject to @lloyddewit approval I suggest it can be merged. Two further comments:
- @shadrackkibet is much more fussy/picky than I am - and this is a compliment in this work. So I would be happy for him to be the main reviewer of these changes.
- I believe @lilyclements has an old draft guide (based on a Windows guide) on good practice in constructing dialogues. I would be delighted if this could be resurrected and some of the points from these types of changes could also be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, no comments
@rdstern Please could you approve (using the GitHub approve mechanism)? then we can merge, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had approved, see message above. But that was before 2 approvals were needed. So I just commented. Insightful comments too!
Now I approve!
@africanmathsinitiative/developers this is ready for review
Fixes #5830
Changes