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

new(text): Add 'shrink-only' option to 'scaleToFit' #1362

Merged
merged 1 commit into from Nov 2, 2021
Merged

new(text): Add 'shrink-only' option to 'scaleToFit' #1362

merged 1 commit into from Nov 2, 2021

Conversation

kyythane
Copy link
Contributor

🚀 Enhancements

  • scaleToFit now takes the option 'shrink-only'

@kyythane
Copy link
Contributor Author

Enhancement for #1360

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

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

@kyythane thanks for the addition! 🙌

This looks great to me. As mentioned in the issue, the docs will be auto-generated from the types, and the only other possible consideration would be to add a demo of the feature to the https://airbnb.io/visx/text demo (code here). if you don't want to, no sweat just wanted to mention 👍 thanks again!

@@ -151,6 +151,38 @@ describe('<Text />', () => {
expect(transform).toBe('matrix(1.25, 0, 0, 1.25, 0, 0)');
});

it("Does not scale above 1 when scaleToFit is set to 'shrink-only'", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for adding tests! looks great 😄

@williaster williaster changed the title [@visx/text] Add 'shrink-only' option to 'scaleToFit' new(text): Add 'shrink-only' option to 'scaleToFit' Oct 26, 2021
@kyythane
Copy link
Contributor Author

kyythane commented Nov 2, 2021

@williaster sorry to bother you if you're busy. I ran into an issue trying to run demos locally. yarn dev:demo errors with

error - ./src/pages/_app.tsx:5:39
Syntax error: Unexpected token, expected ","

  3 | import 'prismjs/themes/prism.css';
  4 | 
> 5 | function MyApp({ Component, pageProps }: AppProps) {
    |                                        ^
  6 |   return <Component {...pageProps} />;
  7 | }
  8 | 

I don't have a ton of time to dig into what's wrong with my local setup. If you're able to give me some potential leads, I'm happy to follow up on them! Otherwise, if you're willing to verify my demo changes on your end I can commit them. If not, I think it would be best to merge the PR without them?

@williaster
Copy link
Collaborator

hey @kyythane sorry for the delay, I was gone at the end of last week. Sorry you're having issues with the dev setup – that looks like some type of Typescript parsing error so I'd ensure you ran yarn && yarn build in the root first before exploring other possible issues.

Maybe we can just keep things simple and merge this as is, then if you'd like to post the diff or open a new PR for your demo changes I'm happy to verify them! For now I'll merge this just so we can get it out/you can use it if you need it without more iteration 😄 thanks again for the contribution!

@williaster williaster merged commit 2b7c810 into airbnb:master Nov 2, 2021
@github-actions
Copy link

github-actions bot commented Nov 2, 2021

🎉 This PR is included in version v2.3.0 of the packages modified 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants