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

Feature: FitText #1342

Merged
merged 20 commits into from
Dec 13, 2024
Merged

Feature: FitText #1342

merged 20 commits into from
Dec 13, 2024

Conversation

ryan-roemer
Copy link
Member

@ryan-roemer ryan-roemer commented Dec 11, 2024

The Basics

Description

Implements FitText component, modeled after the original Spectacle's fit property on various components. Makes ye olde stretchy text to expand dynamically to the width of the slide.

Fixes #1211

Type of Change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

  • Added unit tests for FitText component.
  • Manually checked start:js with normal, exportMode=true and printMode=true
  • Manually checked start:ts.
  • I didn't check one-page as I couldn't quite get the right local reference to the Spectacle ESM entry point. (Might be worth figuring this out and adding a comment in one-page/index.html.)

The Other Stuff

Ok team, go gentle on me as it's been a while since I've done any frontend work. But I've really, really wanted to get this functionality into Spectacle since it was lost many refactors ago.

Also, I'd love to get to have a look here to

The AI angle

I used Jolt AI for three of the commits here, with the following prompts below. @carloskelly13, if you have a moment and desire, would love to get your eyes on this for the Jolt prompts I used generating stuff on your end, how I had to tweak things to get it to work after, and for special bonus points, the actual substance of this code change.

( cc/ @cianclarke too :) )

My thoughts: The initial code gen was good, however the TypeScript wrangling took me over half the time to get it to all work, which it didn't out of the gates. (This, of course, is likely in most part just a skill issue). The tests needed a lot of work to get them to pass. The docs generation was simple and good.

The starting feature request (af5c1cc)

Plan and implement a feature whereby Spectacle can have text that fits the entire width of the containing slide. The alternatives to implement this plan would be:

  • FitText: A new component loosely modeled on the Text component. It has all the same options, it just autofits to the entire width.
  • A fit=true property on components Header, Text.

The requirements are:

  • Text fits to full width in display and printable / export modes.
  • The text fitting should obey normal padding / margin rules within the slide like any other contained Spectacle content.
  • Resizing the browser window continues having the text continue to fit the maximum width of the slide.
  • Any containing behavior currently present (like a Link inside of a Text component) should similar work with the text fit feature.

The optional concerns are:

  • If there is too much text, wrapping to the next line (while still having multiple lines fill the entire width). This could also be skipped, requiring the user instead to choose only what fits per line.

Unit tests (8bc61a5)

Write unit tests for the new FitText component that match the other component tests in typography.test.tsx. Make sure to test all of the following:

  • The FitText text stays at full width even when container and window are resized.
  • The other normal Text props (like color, etc.) correctly work in the FitText component.
  • Any other tests similar to what is tested in Text and Heading that would apply to FitText

Docs (0ea80da)

Update the docs (api-reference.md) to include the new FitText component. Model documentation after the existing Text component, which has all of the same underlying properties besides the scale property added to the FitText component.

Screenshots

Presentation

Screenshot 2024-12-10 at 11 18 02 PM

Export / Print Mode

Screenshot 2024-12-10 at 11 17 48 PM

Copy link

vercel bot commented Dec 11, 2024

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

Name Status Preview Comments Updated (UTC)
spectacle-docs-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2024 8:22am

Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: 412e5b3

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

This PR includes changesets to release 1 package
Name Type
spectacle Minor

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

@ryan-roemer
Copy link
Member Author

Ah, what the heck, I've requested the Copilot auto code reviewer as well...

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 8 changed files in this pull request and generated no suggestions.

Files not reviewed (3)
  • examples/one-page/index.html: Language not supported
  • .changeset/fuzzy-tips-build.md: Evaluated as low risk
  • examples/js/index.js: Evaluated as low risk
Comments skipped due to low confidence (2)

packages/spectacle/src/components/typography.test.tsx:99

  • Ensure the jest.mock implementation correctly resets between tests to avoid potential test interference.
jest.mock('use-resize-observer', () => { return { width: 500, height: 100 }; });

packages/spectacle/src/components/typography.test.tsx:134

  • Review the transform: scale(1) check in the test to ensure it accurately verifies the scaling behavior.
expect(scaledText).toHaveStyle({ transform: 'scale(1)' });
Copy link
Contributor

@carloskelly13 carloskelly13 left a comment

Choose a reason for hiding this comment

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

Yeah I like the idea of it. See the delta between the two containers and then apply a CSS scale. I think this would visually work best with single line headers and such. Like the legacy FitText less great for large paragraphs.

@ryan-roemer
Copy link
Member Author

@carbonrobot @carloskelly13 I ended up removing the use-resize-observer test as I think I was just testing the polyfill/mock itself and not the actual behavior. Let me know if I should dig in more there or if we're fine merging this PR without. Thanks!

@carloskelly13
Copy link
Contributor

@ryan-roemer Those kinds of interactions you are almost testing your mocks more than the code. I think it's fine to merge.

@ryan-roemer
Copy link
Member Author

@carbonrobot I did one last minor tweak to this after Carlos' approval. I'll plan on merging this in the next day or so unless you wanted me to wait on another review pass?

@carbonrobot
Copy link

@ryan-roemer Looks good to me.

@ryan-roemer ryan-roemer merged commit 07667fc into main Dec 13, 2024
3 checks passed
@ryan-roemer ryan-roemer deleted the feature/fit-text branch December 13, 2024 15:06
@github-actions github-actions bot mentioned this pull request Dec 13, 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.

spectacle: Full width / "fit" text feature.
3 participants