-
Notifications
You must be signed in to change notification settings - Fork 88
Fix the autolayout for the pinHeader component having no explicit facingDirection #1565
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| ...props, | ||
| facingDirection: props.facingDirection ?? "right", | ||
| } | ||
| super(propsWithDefaults) |
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.
don't modify props (props are immutable)
Instead, create a function called _getFacingDirection() and force anything using facing direction to use that. This allows much cleaner overrides and preserves the user's definitions (which makes errors and lots of other stuff more clear)
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.
see comment
| import type { BaseSymbolName } from "lib/utils/constants" | ||
|
|
||
| export class PinHeader extends NormalComponent<typeof pinHeaderProps> { | ||
| _getFacingDirection(): "left" | "right" | "up" | "down" { |
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.
sorry
| _getFacingDirection(): "left" | "right" | "up" | "down" { | |
| _getSchFacingDirection(): "left" | "right" | "up" | "down" { |
| // Check for components with _getFacingDirection method (e.g., PinHeader with defaults) | ||
| if ( | ||
| (component as PinHeader)._getFacingDirection && | ||
| typeof (component as PinHeader)._getFacingDirection === "function" |
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 is really incorrect
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.
if ((component as PinHeader)?._getFacingDirection()) {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.
or even better
if (component._getSchFacingDirection()) {then define in the PrimitiveComponent _getFacingSchFacingDirection() { return null }
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.
quick video note- happy to huddle on this one
b73ea54 to
daa1717
Compare
Before
After