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

feat: add scaled text input #1224

Closed
wants to merge 1 commit into from
Closed

feat: add scaled text input #1224

wants to merge 1 commit into from

Conversation

alessey
Copy link
Contributor

@alessey alessey commented Sep 9, 2024

What changed? Why?
Screen Recording 2024-09-09 at 10 21 17 AM

Notes to reviewers

How has it been tested?

Copy link

vercel bot commented Sep 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 9, 2024 2:19pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 9, 2024 2:19pm

Comment on lines +32 to +35
style={{
transform: `scale(${containerScale})`,
width: `${100 / containerScale}%`,
}}
Copy link
Contributor

@cpcramer cpcramer Sep 9, 2024

Choose a reason for hiding this comment

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

We should try to refrain from using inline-styling

Copy link
Contributor

Choose a reason for hiding this comment

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

@cpcramer I think we are forced here to have the inline style. As this is a dynamic CSS story, which I don't think Tailwind has an easy solution for it.

@cpcramer this is also very similar on how CSS animation are built, where you have a basic class, but then use inline style to do crazy animaiton tricks.

@alessey alessey marked this pull request as ready for review September 10, 2024 00:57
import type { TextInputReact } from './TextInput';
import { TextInput } from './TextInput';

export function ScaledTextInput(props: TextInputReact) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some comments that explain when to use ScaledTextInput vs TextInput?

import type { TextInputReact } from './TextInput';
import { TextInput } from './TextInput';

export function ScaledTextInput(props: TextInputReact) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some comments that explain when to use ScaledTextInput vs TextInput?

} else {
setContainerScale(1);
}
}, [props.value]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Open question, could this re-fire too many times if we keep props.value too wide?

export function ScaledTextInput(props: TextInputReact) {
const inputContainer = useRef<HTMLDivElement | null>(null);
const offscreenRef = useRef<HTMLSpanElement | null>(null);
const [containerScale, setContainerScale] = useState(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Help me understand what this number means. Maybe with a line of comment.

return (
<>
<div ref={inputContainer}>
<div
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be done with just 1 <div> instead of 2?

<TextInput {...props} />
</div>
</div>
<span
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dark magic, @alessey tell me more!!! I rememer this from back in the day, can you please add some comments before the return to help explain how we use the combo of inputContainer and offscreenRef to make this magic work.

@alessey the html wizards 🧙

@alessey
Copy link
Contributor Author

alessey commented Sep 10, 2024

closing in favor of increasing the padding between the value and select and keeping the current scroll behavior, cc: @mindapivessa

@alessey alessey closed this Sep 10, 2024
@alessey alessey deleted the alessey/scaled-input branch September 10, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants