-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Components: Integrate "next" Text component #28475
Conversation
69f8c01
to
8096fcd
Compare
8096fcd
to
9eb3b24
Compare
6bef9df
to
f96e1ec
Compare
@@ -31,5 +27,5 @@ export function withNext( | |||
return contextConnect( WrappedComponent, namespace ); | |||
} | |||
|
|||
return forwardRef( CurrentComponent ); | |||
return CurrentComponent; |
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.
We cannot automatically forward the ref of any component (for example, you cannot call forward ref on a styled component like Text
).
Size Change: +1.89 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
@saramarcondes Thank you for working on this one 🙏 !! I gave it some thought. I think we should just replace the current (web) Fortunately, the (current) I poked around, and these seem to be the only places it's being used so far:
I vote that we use it directly 👍 Let me know what you think! If you decide to make the update, I'd be more than happy to help you refactor where it's used + test things out. |
@ItsJonQ Sounds good to me! Do you know how to test the |
f96e1ec
to
2c93ba0
Compare
2c93ba0
to
ba34af3
Compare
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.
🚀 from me! Thank you @saramarcondes .
I paired with Sara this afternoon. In attempting to use only the new <Text />
component, we ran into a potential issue with the Navigation
component.
For that, we think it would be better to go with the adapter strategy for now. A good follow up would be to double check that using the new version of Text
does not break integration with Navigator
.
Description
Integrates the G2 text component.
How has this been tested?
Text
is only used inedit-site
currently and I'm unsure how to test it, but I've added stories for the new component that test the adapter and it appears to work so I think this is safe 🙂Types of changes
New feature (non-breaking change to existing Text component)
Checklist: