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

Adds auto_set_content_size for UIScrollview and subclasses. #196

Merged
merged 6 commits into from Feb 26, 2015

Conversation

@markrickert
Copy link
Member

@markrickert markrickert commented Feb 20, 2015

So that you can add a bunch of things to a scrollview and then call auto_set_content_size and it will not change frame but automatically set the contentSize based on the scroll view's contents (with optional padding)

So that you can add a bunch of things toa scrollview and then call auto_set_content_size and it will not change frame but automatically set the contentSize based on the scroll view's contents (with optional padding)
@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 20, 2015

Once #195 is merged, I'll modify this PR to pull out the subview size calculation into its own method so that it's not repeated in both auto_set_content_size & resize_to_fit_subviews

markrickert added 2 commits Feb 20, 2015
* master:
  Allows you to specify right and bottom padding for resize_to_fit_subviews

Conflicts:
	spec/position.rb
DRY
@GantMan
Copy link
Member

@GantMan GantMan commented Feb 20, 2015

This is awesome.

I'd like to bring up a major issue that is starting to rear it's head. @twerth your feedback on this idea would be most appreciated.

this is auto_set_content_size
on labels we have resize_to_fit_text alias size_to_fit
and on UIViews we have resize_to_fit_subviews

Since Mark's code here is not actually resizing a scrollview, should we possibly add aliases to the others, taking the auto_set prefix?

It would be nice to add some conformity in this

We remember one method that changes depending on the object. I'm going to suggest auto_set_content_size and auto_set_frame_size as the 2 options.

@twerth
Copy link
Member

@twerth twerth commented Feb 20, 2015

I agree to having some conformity in these names.

IMO, auto_set_content_size means it will automatically set content size when something happens. That's the way I read it. I'd rather that be named something that suggests it's going to happen right now and only right now.

@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 20, 2015

I see your point. I picked "auto_set" because it automatically sets it based on the subviews. I'm open to a different name though 👍

@twerth
Copy link
Member

@twerth twerth commented Feb 20, 2015

I'd actually like to eventually have an "auto_set" where it does automaticallyl set it when something changes. But that's a bigger feature.

Imagine you do this st.auto_resize_to_fit_children = true and anytime a child's frame changes it sets the parent if needed and applicable

@twerth
Copy link
Member

@twerth twerth commented Feb 20, 2015

It's long, but how about: resize_content_to_fit_subviews assuming that's exactly what it does.

I'm a big believer in "make a method name as short as possible and be completely clear, and no shorter"

@GantMan
Copy link
Member

@GantMan GantMan commented Feb 20, 2015

Also, resize_to_fit_subviews will need documentation on the site along side with whatever the name ends up being.

I'm 👍 with resize_content_to_fit_subviews.

Playing with 🔥
What about view_refit and content_refit ?
It meets all needs. Name says what it is, and it's short/clean.

@twerth
Copy link
Member

@twerth twerth commented Feb 20, 2015

I personally don't like view_refit, it's a bit unclear, but I'm curious what Mark thinks.

@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 20, 2015

I'm not a fan of the shorter name. not descriptive enough. It's a complex task we're trying to describe in just a few words.

if we're gonna go with resize_content_to_fit_subviews we should also change the name of the other method to resize_frame_to_fit_subviews, no?

@GantMan
Copy link
Member

@GantMan GantMan commented Feb 20, 2015

/giphy Take my ball and go home.

@twerth
Copy link
Member

@twerth twerth commented Feb 21, 2015

Yeah, that makes sense @markrickert. I like that. Leave the alias for now though for backwards compatibility.

markrickert added 2 commits Feb 23, 2015
…tent_size

* 'master' of github.com:infinitered/rmq:
  Allow setting keyboard_appearance with a symbol
  use Numeric#close? to get rect specs to pass on device
  make rmq.device.simulator? spec work when run on the device
To fit conventions agreed upon.
@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 23, 2015

Cool. I've renamed the method as well as changing the frame setter and creating an alias to the old method.

@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 24, 2015

Tests passed :)

Allows you to pass :only_width and :only_height so that things aren't resized that you don't want to be :)
@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 26, 2015

New feature for resize_frame_to_fit_subviews:

Allows you to pass :only_width or :only_height so that things aren't resized that you don't want to be :)

Useful for when you have views inside a uiview that don't extend all the way to the right and you only want to resize the height of the frame.

@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 26, 2015

@GantMan @twerth i think this one is ready to go. 3 new tests with 26 new assertions!

I'll write documentation once it's merged and a new version is released.

@GantMan
Copy link
Member

@GantMan GantMan commented Feb 26, 2015

Mark adding tests:

nice tests bro

GantMan added a commit that referenced this pull request Feb 26, 2015
Adds auto_set_content_size for UIScrollview and subclasses.
@GantMan GantMan merged commit 3d8fcc2 into master Feb 26, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@GantMan GantMan deleted the auto_set_content_size branch Feb 26, 2015
@markrickert
Copy link
Member Author

@markrickert markrickert commented Feb 26, 2015

lol

@GantMan
Copy link
Member

@GantMan GantMan commented Mar 14, 2015

Hey @markrickert I just searched for this today on the docs. Let me know if you want any help adding them! I need to go through anyway and add docs for my stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants