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

Add monospace prop to textinput #1935

Merged
merged 5 commits into from Mar 11, 2022
Merged

Add monospace prop to textinput #1935

merged 5 commits into from Mar 11, 2022

Conversation

pksjce
Copy link
Collaborator

@pksjce pksjce commented Mar 8, 2022

Describe your changes here.

Added a monospace prop to TextInput component

image

TextInput now accepts a prop called monospace which is boolean. When this prop is present, the textinput changes font-family to fonts.mono. All other functionality remains same.

Closes https://github.com/github/primer/issues/757

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@pksjce pksjce requested review from mperrotti, rezrah and a team March 8, 2022 10:23
@changeset-bot
Copy link

changeset-bot bot commented Mar 8, 2022

🦋 Changeset detected

Latest commit: c5f327e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 63.2 KB (+0.03% 🔺)
dist/browser.umd.js 63.56 KB (+0.05% 🔺)

docs/content/TextInput.mdx Outdated Show resolved Hide resolved
## Monospace text input

```jsx live
<TextInput monospace aria-label="personal access token" name="pat" placeholder="github_pat_abcdefg123456789" />
Copy link
Contributor

Choose a reason for hiding this comment

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

after checking out the deployment preview I realised my earlier suggestion wasn't quite right, my bad.

I think you should switch placeholder to value so it looks more like product version. Would be nice to get the CheckIcon in there too!

Suggested change
<TextInput monospace aria-label="personal access token" name="pat" placeholder="github_pat_abcdefg123456789" />
<TextInput monospace leadingVisual={CheckIcon} aria-label="personal access token" name="pat" value="ghp_abcdefg123456789" />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done 👍

${props =>
props.monospace &&
css`
font-family: ${get('fonts.mono')};
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to tweak the size here too? the new type face looks a bit larger than usual inputs so may look odd adjacent to them. May just be the fixed-pitch of the the monospace font playing tricks on my eyes though. cc. @mperrotti
Screenshot 2022-03-08 at 12 43 37

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I feel the size issue too. Would appreciate help from @mperrotti on this design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Took another detailed look and it may just be how monospace fonts look. Its the same size as the one on the developer settings > PAT generator page. I think I'll go ahead with what we have.

Copy link
Contributor

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

✅ with some minor comments / questions

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.

None yet

2 participants