[Free Response Widget] Add ability to customize placeholder #2297
Conversation
|
Size Change: +102 B (+0.01%) Total Size: 874 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (a9cabf2) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2297If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2297 |
|
Do I need to add a changeset if merging into a feature branch? |
@rgpass The important thing is that there's a changeset when landing into main and that changeset includes all the packages that changed. If a package is changed and it's not included in the changeset, it might not get a new version in We don't use feature branches a lot in Perseus, so that's why the changeset check isn't smarter about looking at your base branch. IMO it's fine to land a PR without a changeset as long as you're not landing into main. You can also do |
| >; | ||
|
|
||
| const defaultWidgetOptions: FreeResponseDefaultWidgetOptions = { | ||
| placeholder: "Please provide response here", |
There was a problem hiding this comment.
I think this is probably the right way to do this. My initial gut concern was that this needed to be marked for translation in Perseus (vs CP), but I don't think that's correct.
- "Please provide response here" will be the default in the editor and content creators can write their own string
- On publish this string will get sent to CP and their parser will need to know that this is a translatable string
- It'll get published and we'll receive the translated string when rendering
That seems fine. If we were storing the placeholder as something like string | null | undefined in the schema and defaulting to "Please provide response here" at render (vs at publish) then we would need that string in perseus/src/strings.ts.
Going this path does allow for less flexibility in the future if we want to change the default string:
- If we use a default placeholder when publishing (what this PR is doing), we have to run a backfill to update existing content
- If we use a default placeholder when rendering, we can change the string without running a backfill since we could just change the string in
perseus/src/string.tsand request a new translation for that one string
Hmm...now that I've typed all this up I'm wondering if this should change. @benchristel what do you think?
There was a problem hiding this comment.
I don't know the product context for this feature, so it's hard for me to say what the right choice is. TBH, I don't quite understand why content creators want the ability to customize the placeholder.
In the absence of more information, I would lean toward the current approach of defaulting the placeholder at content-creation time. That way, even if we change the default in the future, learners looking at old content will always see a placeholder that makes sense in context.
There was a problem hiding this comment.
That way, even if we change the default in the future, learners looking at old content will always see a placeholder that makes sense in context.
That was our thinking as well.
Given that, are there any changes needed?
I think I'd prefer adding a changeset to each individual PR before landing into the feature branch. That way, each changeset refers to one atomic change, and it's easier to trace the changes through the git history and PRs. I think that workflow will work well with our tooling. AFAIK the changesets check in CI just wants to see a changeset for each package that differs from the base branch. So we won't have to add extra changesets before landing the feature branch into |
…ity to customize the placeholder text in a Free Response widget
Summary:
This PR adds the ability to set the placeholder text in a Free Response Widget component.
Issue: LIT-1668
Test plan:
NOTE: The screenshots use something I typed in. The default value is "Please provide response here"