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

fix: Input now support refs #123

Closed
wants to merge 25 commits into from
Closed

fix: Input now support refs #123

wants to merge 25 commits into from

Conversation

tibuurcio
Copy link
Collaborator

Summary

This PR adds support for using Refs in our Input component the same way Antd's native Input supports it. With this we are able to support implementations that need to control input focus.

Implementation was based on the one from Antd

Testing Plan

  • Was this tested locally? If not, explain why.
  • Run storybook and run tests
Aquarium._.Data.Entry._.Input.-.With.Focus.Management.Storybook.-.Google.Chrome.2024-03-01.19-04-06.mp4

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

@tibuurcio tibuurcio self-assigned this Mar 1, 2024
@tibuurcio tibuurcio changed the title fix: Input supporting refs by using forwardRef fix: Input now support refs Mar 1, 2024
@tibuurcio tibuurcio changed the base branch from main to next March 1, 2024 22:05
.prettierrc Outdated
@@ -2,5 +2,6 @@
"semi": false,
"singleQuote": true,
"arrowParens": "avoid",
"printWidth": 120
"printWidth": 120,
"bracketSameLine": true
Copy link
Collaborator Author

@tibuurcio tibuurcio Mar 1, 2024

Choose a reason for hiding this comment

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

With our printWidth size it doesn't make much sense to keep the default for bracketSameLine as well (false).

This is the change this config makes:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This made some changes to files outside of the scope of this PR. I'm thinking on removing this and introducing it in a new PR 🤔

Let me know what you all think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea best to have linting updates in a separate pr where possible

@tibuurcio tibuurcio changed the base branch from next to main March 14, 2024 19:34
@tibuurcio
Copy link
Collaborator Author

I'm gonna cherry pick this PR cause the merging of next messed it up.

@tibuurcio tibuurcio closed this Mar 14, 2024
tibuurcio and others added 7 commits March 15, 2024 15:38
…input-forward-ref

# Conflicts:
#	src/assets/svg/alicorn.svg
#	src/components/icons/index.ts
#	src/components/index.ts
#	src/components/navigation/GlobalNavigation/WorkspaceSelector/WorkspaceSelector.tsx
#	src/components/navigation/GlobalNavigation/WorkspaceSelector/WorkspaceSelectorContent.tsx
#	src/components/navigation/GlobalNavigation/global-navigation.css
#	src/utils/utils.spec.ts
@tibuurcio tibuurcio reopened this Mar 15, 2024
@tibuurcio tibuurcio requested a review from jared-dickman March 20, 2024 14:20
Input.Search = AntInput.Search
Input.TextArea = AntInput.TextArea

export { Input, type InputRef, type IInputProps }
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious why we need an index for Input only, cant this go in src/components/index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm testing these to see if we can use the already present index

@@ -17,10 +18,6 @@ import { useEffect } from 'react'
import { useMemo } from 'react'
import { debounce, hasImageAtSrc } from 'src/utils/utils'
import { getInitials } from 'src/utils/utils'

// TODO: Need to make our Input component comply with forwardRef to be able to import it from src/components
Copy link
Collaborator

Choose a reason for hiding this comment

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

praise TODONE!

Comment on lines -16 to -21
Input.Group = AntInput.Group
Input.Password = AntInput.Password
Input.Search = AntInput.Search
Input.TextArea = AntInput.TextArea
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Try to keep this in the Input file if we can.

@tibuurcio
Copy link
Collaborator Author

I messed up this PR somehow, gotta learn that git stuff properly 🤦🏼‍♂️
image
Gonna open a new one.

@tibuurcio tibuurcio closed this Mar 22, 2024
@jared-dickman
Copy link
Collaborator

@tibuurcio spoilers - even linus doesnt "knows git properly" 😂

@jared-dickman jared-dickman reopened this Mar 22, 2024
@tibuurcio
Copy link
Collaborator Author

#169 fixed

@tibuurcio tibuurcio closed this Mar 22, 2024
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.

2 participants