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

improved editbox sizing #37

Merged
merged 6 commits into from
Feb 21, 2016
Merged

Conversation

KuroiLight
Copy link
Contributor

changes how editboxes are sized, labels now on top instead of beside editbox; this should make editing longer texts easier. Text should also be aligned better.

-Terrillyn

@sirinsidiator
Copy link
Owner

The code looks good, but you need to make it opt in as it will cause unexpected changes in addons that already use the editbox.
I suggest you add a boolean "isExtraWide" to the properties.

@merlight
Copy link
Contributor

merlight commented Feb 4, 2016

How about:

if type(editboxData.isMultiline) == "number" then
  if editboxData.isMultiline <= 1 then -- new single-line style
  else -- new multi-line style, use the number to set height
  end
else -- old behaviour
end

@sirinsidiator
Copy link
Owner

I'm not really a fan of multipurpose properties that do not match their name.
For me the name isMultiline clearly indicates that it is a boolean, so it should not be used to contain a number.
Just add a height property if you need one.

@KuroiLight
Copy link
Contributor Author

makes use of isExtraWide

@sirinsidiator
Copy link
Owner

I just merged all 4 pull requests locally and the result looks good except for two issues:

The half width single line edit box is now on the same line as the label, which causes overlapping if the label is too long and there is also an empty space below the shown controls that was not there before.

@KuroiLight
Copy link
Contributor Author

The spacing issue is related to the changes in #36 , I'm pretty sure its caused by the offsets set when creating the controls, I'll take a look and commit to #36.

@KuroiLight
Copy link
Contributor Author

Fixed typo, editboxData.isHalfWidth -> control.isHalfWidth (this fixes a container sizing issue)
Made warning icon show up above the topright corner of the editbox when isExtraWide is enabled to avoid overlapping
Simplified structure of if's
screenshot_20160207_224754

@sirinsidiator
Copy link
Owner

I just pulled your latest changes and get the following error:

user:/AddOns/LibAddonMenu-2.0/LibAddonMenu-2.0/controls/editbox.lua:103: function expected instead of nil
stack traceback:
    user:/AddOns/LibAddonMenu-2.0/LibAddonMenu-2.0/controls/editbox.lua:103: in function 'LAMCreateControl.editbox'
    user:/AddOns/AutoInvite/ui/ai_enabled_fragment.lua:46: in function 'AutoInviteUI:CreateEnabledFragment'
    user:/AddOns/AutoInvite/AutoInviteUI.lua:34: in function 'AutoInviteUI.init'
    user:/AddOns/AutoInvite/AutoInvite.lua:171: in function '(anonymous)'

AFAIK only AutoInvite uses the settings controls outside of the settings menu (like described in the documentation) and passes a parent that is not a panel, but I would still like to support this case, as minor versions should not break compatibility. I did that once and got a really angry pm as a result. :P

Something like local MIN_WIDTH = parent.GetWidth and parent:GetWidth() / 10 or 0 should do the trick.

Other than that it looks good now.

sirinsidiator added a commit that referenced this pull request Feb 21, 2016
@sirinsidiator sirinsidiator merged commit 3742be4 into sirinsidiator:master Feb 21, 2016
@sirinsidiator sirinsidiator added this to the r19 milestone Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants