Add 'Take Sourcegraph to your [code host]' popover #14256
Conversation
tjkandala
added
RFC-221
|
In https://github.com/sourcegraph/sourcegraph/pull/14261 I made it so it shows the toast on your 3rd day of activity, not your 1st. Not sure how you're doing it here, but you should make it so this doesn't show up immediately the very first time they load Sourcegraph. |
|
We're removing the toast as part of RFC 221 in favor of inline alerts after the user has seen 2 hovers. That's a good idea though; cc @sourcegraph/web @AlicjaSuska Should we check |
|
This popup is displayed after clicking on 'view on GitHub'. I think it doesn't make sense to add the day count as criteria here. I think it makes sense to show this information even if someone clicks 'view on [code host]' for the first time. |
|
@AlicjaSuska maybe we can include the criterium "Has seen at least 2 hovers" too with the same reasoning as for the alert? |
|
@felixfbecker let's add the '2 hovers' criteria, you're right. |
|
If the user clicks "Install the Sourcegraph browser extension" from the alert, but doesn't install the extension, should we disable the popover for the 'View on code host button'? They're both dependent on the user seeing 5 hovers now, so it might be annoying to see the popover after already looking to, but not installing, the extension. |
tjkandala
changed the title
|
| } | ||
|
|
||
| /** | ||
| * A component that is displayed in the same way, regardless of whether it's a link (with a | ||
| * destination URL) or a button (with a click handler). | ||
| * | ||
| * It is keyboard accessible: unlike <Link> or <a>, pressing the enter key triggers it. Unlike | ||
| * <button>, it shows a focus ring. | ||
| * It is keyboard accessible: unlike `<Link>` or `<a>`, pressing the enter key triggers it. |
There was a problem hiding this comment.
thought: I'm starting to question the validity of the entire LinkOrButton/ButtonLink component. This comment stated that <button>s don't show a focus ring, which is false - the buttons in our app all have focus rings. But now I just verified that hitting enter when a link is focused also triggers navigation and click handlers in Chrome, so that statement seems to be false too, leaving no reason to use this component.
The only case where it would not do anything is if the link only had onClick, but in that case it should not be a link, it should be a <button>. It seems like for our use cases we should always use <button> or <a>/<Link> directly and avoid this component, potentially remove it.
cc @sourcegraph/frontend-engineers
Codecov Report
@@ Coverage Diff @@
## main #14256 +/- ##
==========================================
- Coverage 52.70% 46.97% -5.74%
==========================================
Files 1030 1550 +520
Lines 65273 78991 +13718
Branches 2427 6897 +4470
==========================================
+ Hits 34402 37105 +2703
- Misses 27194 38166 +10972
- Partials 3677 3720 +43
|
|
Update: Replaced all (2) modals in the web app with Reactstrap popoverKeyboard navigation
Fade in and visuals are same as in original post. Reach popoverKeyboard navigation
No fade in. Popover visuals
SummaryReactstrap's popover is easier to visually customize, but I don't think it's meant to be used with interactive elements. Reach adheres to the ARIA spec, but is hard to position when compared to Popper's I'll spend a little more time trying to get Reach's popover to align with the design. If that doesn't work out, I propose that we use reactstrap's popper combined with the focus-trap library (or react-focus-lock, which was added to the web app by Edit: Went with reactstrap + react-focus-lock, the popover works like the original implementation now. |
|
@tjkandala looks like you did good research on the best solution for popovers! If we're using Reactstrap now, does that mean we also need to add an import to the Bootstrap styles and they should "just work"? (I see that we still have custom CSS for the popover despite using Reactstrap, and that the arrow still doesn't work) |
Replaced custom CSS with Bootstrap styles, which fixed the arrow. Weirdly, importing Bootstrap styles changed the way that the offset works. No offset
Offset "0, -4" (Same as original post)
|
There was a problem hiding this comment.
Thank you for this work @tjkandala I finally could test it after your help 🎉
Improvements that are needed:
- popup should appear for users who have 3 hovers, not 5 (as it is now)
- pointer styles are not aligned with the design. If it's a very difficult fix we can also remove the pointer entirely to speed things up.
- in dark mode, there are light points in the corners of the popover. I'm not sure what are those.
|
update: @AlicjaSuska. Links also open in new tabs when the user originally middle clicked 'Go to [code host]', and docs always open in new tab. |
| } | ||
|
|
||
| & .arrow::after { | ||
| border-bottom-color: var(--body-bg) !important; |
There was a problem hiding this comment.
question: shouldn't this be set in global-styles for all popovers? Or perhaps there is just a Bootstrap Sass var we can set for this? Usually we need to set some Sass variables for every Bootstrap component when start using it to make it theme-aware, and this PR is the first one to use popover, so that's likely
There was a problem hiding this comment.
Implemented this change. However, I found out that other popovers in our application broke once we imported Bootstrap's popover styles, probably since they were built without Bootstrap (css) in mind. I set $popover-max-width to auto. Do you think this is the right change?
There was a problem hiding this comment.
We obviously can't break other popovers, so if that change fixes it, that sounds right. I wasn't aware we actually used the class elsewhere given we had not imported it. I also just found these styles:
They can probably be removed, and that comment looks like it doesn't apply if we set the variables correctly. The arrow also seems to be an element you have to explicitly render in the DOM, so that can just be omitted if we don't want it for certain popovers (like the rev switcher).
@AlicjaSuska this was something we discussed when we hacked on this, 3 felt very low. Do you feel strongly about 3? |
tjkandala
force-pushed
the
tj/code-host-popup
branch
from
7393601
to
a2412e4
Compare
Display an alert on first render of a repo page after the user has seen 3 hovers. Add a popover when the user first clicks on 'View on [code host]'.
Closes #14199. Part of RFC 221
GoToCodeHostActionto function componentpopuppopoverInteraction
Popover fade in on click
Keyboard navigation, "Remind me later"
Popover visuals
@AlicjaSuska The "arrow" is a square rotated 45 degrees and 50% hidden by the popover. This approach is good for responsive popovers since it's mostly the same styles for each direction, but it doesn't align 100% with the design. I'll revisit the arrow after finishing the remaining issues.