-
Notifications
You must be signed in to change notification settings - Fork 528
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 #4460: Add Wiki documentation for adding the spotlight experience to any element #4943
Conversation
Additionally, here is this link to the original Spotlight Wiki doc: https://docs.google.com/document/d/1houcOoZLbTgdwnL1HnmocolEjP6exfY-R_qsNbYPY7Y/edit# |
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.
Thanks @JishnuGoyal! I think the filename is incorrect. Also we need to add this to the _Sidebar.md file so that it can be found on the side.
Besides that, since I've already reviewed the contents of this file in detail, I'm actually going to defer to @seanlip for the PR review since he's also broadly going to be reviewing wiki pages moving forward.
@JishnuGoyal please assign to @seanlip once the comments above are addressed. |
Hey @BenHenning, how exactly should I make sure this file is shown in the sidebar? |
PTAL @seanlip , thanks! |
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.
Thanks @JishnuGoyal! Took a full pass, PTAL.
Re including this page in the sidebar, @BenHenning's already given you a pointer -- please add it to the relevant place in _Sidebar.md
. Thanks!
Please take another pass at this @seanlip , thanks! |
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.
Thanks @JishnuGoyal, I think the page content looks fine. But I think you missed @BenHenning's comment from before. Could you please:
(a) Add this page to the _Sidebar.md file so that users can get to it
(b) Make sure this page is named in the same way as others in the wiki (no spaces in the name, use hyphens)
Once you've done that, this PR can be merged. Thanks!
Updated the file name and added the same to the sidebar. @seanlip PTAL, thanks! |
Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks! |
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.
Thanks! LGTM.
Unassigning @seanlip since they have already approved the PR. |
I have updated this branch. @BenHenning PTAL, thanks! |
Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks! |
Merged. Thanks @JishnuGoyal! |
Explanation
Fixes #4460
This PR adds a new page to the Wiki explaining how Spotlights should be used, how new ones should be added and how should they be tested.
Essential Checklist