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

feat(preview): Extend previewExperiences #2886

Closed

Conversation

robertjk
Copy link

@robertjk robertjk commented May 8, 2022

Description

Extend <ContentPreview> component, so that Persistent Onboarding Box Edit tooltip can be displayed on <AnnotationsControls> in box-content-preview:

  • Rename previewExperiences property to previewDependencies
  • In addition to experiences add 2 new properties in previewDependencies:
    • isLocaleLanguageEnglish
    • onPersistentOnboardingTooltipBack
  • Add onPrevious() callback to Persistent Onboarding experience

@robertjk robertjk requested review from jstoffan, ConradJChan and a user May 8, 2022 17:44
@robertjk robertjk requested a review from a team as a code owner May 8, 2022 17:44
@CLAassistant
Copy link

CLAassistant commented May 8, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Robert J. Kusznier seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@robertjk robertjk force-pushed the annotations-tooltip-current-way branch 2 times, most recently from 27503fb to a000603 Compare May 11, 2022 15:56
@robertjk robertjk changed the title feat(preview): Change previewExperiences to previewDependencies feat(preview): Extend previewExperiences May 11, 2022
@robertjk robertjk force-pushed the annotations-tooltip-current-way branch from a000603 to 7088e00 Compare May 11, 2022 16:01
@robertjk robertjk requested a review from a team as a code owner May 11, 2022 16:01
@@ -105,7 +105,10 @@ type Props = {
onNavigate: Function,
onVersionChange: VersionChangeCallback,
previewExperiences?: {
[name: string]: TargetingApi,
experiences: {
Copy link

Choose a reason for hiding this comment

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

Why is this change necessary? Would previewExperiences instead of previewExperiences.experiences be simpler? Also, can we keep it generic and instead modify TargetingApi to have the optional onPrevious method?

Copy link
Author

Choose a reason for hiding this comment

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

Why is this change necessary?

It's not necessary, but IMO it results in safer types. See explanation here: box/box-content-preview#1445 (comment)

Would previewExperiences instead of previewExperiences.experiences be simpler?

That's a good point. I forgot to revert that extra experiences property. Let me do it.

Also, can we keep it generic and instead modify TargetingApi to have the optional onPrevious method?

It's the same discussion we already started in EndUserApp. To not duplicate it, let's keep the conversation there.

@robertjk robertjk force-pushed the annotations-tooltip-current-way branch from 7088e00 to 757db89 Compare May 12, 2022 15:19
@robertjk robertjk closed this by deleting the head repository May 21, 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.

None yet

2 participants