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

TextArea: Add support for Tags #1315

Merged
merged 21 commits into from
Jan 8, 2021
Merged

TextArea: Add support for Tags #1315

merged 21 commits into from
Jan 8, 2021

Conversation

ohariz
Copy link
Contributor

@ohariz ohariz commented Dec 17, 2020

Same as #1306 but for TextArea

Tag component
Doc
Figma

Test Plan

Screen Shot 2020-12-17 at 3 31 45 PM
Screen Shot 2020-12-17 at 3 31 56 PM
Screen Shot 2020-12-17 at 3 32 20 PM
(the weird scrollbar overflow issue is unrelated to these changes, and I couldn't find a quick fix)

@ohariz ohariz requested a review from a team as a code owner December 17, 2020 20:38
@netlify
Copy link

netlify bot commented Dec 17, 2020

✔️ Deploy preview for gestalt ready!

🔨 Explore the source changes: dd9dcd8

🔍 Inspect the deploy logs: https://app.netlify.com/sites/gestalt/deploys/5fdbc1dbb4403e000897c85d

😎 Browse the preview: https://deploy-preview-1315--gestalt.netlify.app

@netlify
Copy link

netlify bot commented Dec 17, 2020

✔️ Deploy preview for gestalt ready!

🔨 Explore the source changes: 05d9da6

🔍 Inspect the deploy logs: https://app.netlify.com/sites/gestalt/deploys/5ff8a6900586b900078c3b24

😎 Browse the preview: https://deploy-preview-1315--gestalt.netlify.app

.unstyledTextArea {
composes: absolute left0 top0 right0 bottom0 from "./Layout.css";
height: 100%;
padding: 6px 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We're actually in the process of making Spacing tokens, so in the meantime we should probably stick to our "boint" system (4px = 1 boint), like so:

padding: calc(var(--g-boint) * 4)

Also, 6px will not be part of the new system, can this go up to 8 or down to 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 6px are there to align the text with the tags. I can't really do any centering hackery because the <textarea> needs to have a 100% height. This is what it would look like with 4px:
Screen Shot 2020-12-18 at 10 17 13 AM
Screen Shot 2020-12-18 at 10 17 22 AM
I can change it if @ashleyseto is ok with it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue right now is the text in the input is larger than the text in the tags. Is that correct? I'm not really sure how to resolve this right now. If we want to keep to 4px for now to not add odd spaces, we can. I think most of my spacing comments were around the internal part of the tag so that doesn't look weird.

packages/gestalt/src/TextArea.css Outdated Show resolved Hide resolved
packages/gestalt/src/TextArea.css Outdated Show resolved Hide resolved
packages/gestalt/src/TextField.css Outdated Show resolved Hide resolved
@AlbertCarreras AlbertCarreras added the minor release Minor release label Jan 7, 2021
Copy link
Contributor

@ayeshakmaz ayeshakmaz left a comment

Choose a reason for hiding this comment

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

Looks great 🎉

@ayeshakmaz ayeshakmaz changed the title TextArea: add tag support TextArea: Add support for Tags Jan 8, 2021
@mergify mergify bot merged commit 37bf8c2 into pinterest:master Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants