Skip to content

Conversation

@jd-carroll
Copy link
Contributor

First cut of allowing custom size modes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this option needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolveSizeMode is called while iterating through the list of input params for the customized function. If it returns a value, then we know we need to retrieve the value for one of absolute, relative, or render. If it doesn't return a value, then we use the value from the array as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not real satisfied with this... 5 input params is a lot so I am definitely open to other suggestions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone could share a general framework / thought process for testing the different modes, I can take a stab at writing the tests. We definitely need these as there was already one issue #354 & #355

@jd-carroll
Copy link
Contributor Author

I am now utilizing this code in my local repo, if this could have a final review that would be appreciated.
May need to tweak a couple of things:

  • Passing fn + params into Size component (L1065)
  • Creating new array for resolving dependencies (L360)
  • Scope of custom mode fn (L370)

@alexanderGugel

@michaelobriena
Copy link
Member

@jd-carroll I am a bit torn on this.

While I think this is really cool and adds flexibility I wonder if custom size mode functionality should be part of the Node or if it should just be a component attached to a Node that manages the size. I think the counter would be that Relative and Render size should then be be components as well.

Anyone else have thoughts on this? Need to think on this for a bit.

@michaelobriena
Copy link
Member

@jd-carroll either way, thanks for introducing the idea!

@alexanderGugel
Copy link
Member

After a lot of back and forth on this, I'm against this change. At least at this point in time. I agree with @michaelobriena that this should be a component. In fact, I would even argue that every size mode should be a component and pixel size should be the only core size mode.

After all, I don't think this is something we currently want/ need and it introduces unnecessary complexity.

@jd-carroll Thanks for the PR though, but I don't think this is something we currently want/ need.

@michaelobriena Can we close this?

@jd-carroll jd-carroll closed this Dec 31, 2023
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.

4 participants