-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix: DIA-1767: Move FE Tour component to LSO #6907
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for label-studio-docs-new-theme canceled.
|
✅ Deploy Preview for heartex-docs canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6907 +/- ##
========================================
Coverage 76.94% 76.94%
========================================
Files 175 175
Lines 14139 14139
========================================
Hits 10879 10879
Misses 3260 3260
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…code without coupling
…tected as a module upstream
const setTourViewed = useCallback( | ||
(name: string, isSkipped: boolean, interactionData: Record<string, any> = {}) => { | ||
// TODO: currently we don't have per-tour complete state, so we just update the global state | ||
updateProductTourState(api, name, isSkipped ? "skipped" : "completed", interactionData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateProductTourState
returns a promise that we're not awaiting here or in restartTour
which means we fire the request and dont care if it resolves as expected or not - is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, it depends on the particular usage of this function. Right now, we just need to set flag on backend to prevent tour reappearing next time. But this can be done async, no reason for waiting. But I'll put it inside updateProductTourState for clarity
tooltip: { | ||
width: "468px", | ||
}, | ||
options: { | ||
primaryColor: "var(--primary_link)", | ||
textColor: "var(--sand_900)", | ||
overlayColor: "rgba(0, 0, 0, 0.5)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we define the width and the overlayColor using a css variable instead of hard setting them here? this will make future theming and white labeling efforts easier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only takes in these style props, doesn't appear to have that option unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to dig more into that 🙁
Co-authored-by: yyassi-heartex <[email protected]>
PR fulfills these requirements
[fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made
ex.fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
Change has impacts in these area(s)
(check all that apply)
Describe the reason for change
(link to issue, supportive screenshots etc.)
What does this fix?
(if this is a bug fix)
What is the new behavior?
(if this is a breaking or feature change)
What is the current behavior?
(if this is a breaking or feature change)
What libraries were added/updated?
(list all with version changes)
Does this change affect performance?
(if so describe the impacts positive or negative)
Does this change affect security?
(if so describe the impacts positive or negative)
What alternative approaches were there?
(briefly list any if applicable)
What feature flags were used to cover this change?
(briefly list any if applicable)
Does this PR introduce a breaking change?
(check only one)
What level of testing was included in the change?
(check all that apply)
Which logical domain(s) does this change affect?
(for bug fixes/features, be as precise as possible. ex. Authentication, Annotation History, Review Stream etc.)