-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement walkthrough state provider #3659
Implement walkthrough state provider #3659
Conversation
6cec0fd
to
64c2313
Compare
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 the approach context-key overall looks ok, but I think we can probably simplify WalkthroughStateProvider
some. I've left some overall comments to take a look at.
NOTE: I spent a bit of time to re-enable a lot of feature changes to review this locally. Let's not do that in the future.
export function entries<TKey extends PropertyKey, TVal>(o: Partial<Record<TKey, TVal>>): [TKey, TVal][] { | ||
return Object.entries(o) as [TKey, TVal][]; | ||
} | ||
|
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.
Objects already have strong typing based on the object's shape, so this shouldn't be needed.
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.
Native Object.entries typing doen't work correctly with constant/enum keys, it always considers that keys are strings
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.
This worked fine for me:
(Object.entries(this.walkthroughByTracking) as [TrackedUsageKeys, WalkthroughContextKeys][])
"provider-jira": 61747 | ||
"provider-jira": 61747, | ||
"play-button": 61748, | ||
"rocket-filled": 61749 |
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 don't see rocket-filled
used anywhere
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.
56e5711
to
d9feb2b
Compare
eb450ed
to
5b4fbd3
Compare
- fix review notes - add contents to the left part - add mock contents to the right part - add feature flag
Co-authored-by: Keith Daulton <keith.daulton@gitkraken.com>
a83c128
to
17d3152
Compare
37b9dfa
to
663ebbd
Compare
Description
The feature is disabled. To enable use setting flag
gitlens.test.newWalkthrough
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses