Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Feb 3, 2021

Overview

This PR:

  1. Adds product research sign-up page. See screenshot below. The sign-up link directs to https://share.hsforms.com/1tkScUc65Tm-Yu98zUZcLGw1n7ku
  2. Gates product research sign up page behind admin configuration settings. Please check that I did this right 😀
  3. Adds jest-dom matchers for use with testing-library and update tests to use these where suitable. These are more declarative and also provide additional tests. For example, toBeVisible checks the existence of the element but also ensure that it is not hidden from the user in someway (e.g. display: none). I have created an eslint plugin PR to our shared config to encourage the use of these with testing-library: feat: add plugin-jest-dom eslint-config#188.

Screenshots

image

Progress

  • Documentation text/screenshots updated (if relevant)
  • Approved by a frontend engineer (if touching frontend code)
  • Approved by a backend engineer (if touching backend code)
  • Approved by a designer (if it touches the UI)

Closes https://github.com/sourcegraph/sourcegraph/issues/17646
Closes https://github.com/sourcegraph/sourcegraph/issues/17647

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #17945 (449cd89) into main (32dda4a) will decrease coverage by 0.01%.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##             main   #17945      +/-   ##
==========================================
- Coverage   51.03%   51.01%   -0.02%     
==========================================
  Files        1739     1737       -2     
  Lines       86848    86987     +139     
  Branches     7916     7750     -166     
==========================================
+ Hits        44323    44378      +55     
- Misses      38641    38728      +87     
+ Partials     3884     3881       -3     
Flag Coverage Δ
go 49.94% <0.00%> (-0.02%) ⬇️
integration 30.37% <66.66%> (+0.02%) ⬆️
storybook 30.13% <0.00%> (-0.34%) ⬇️
typescript 53.59% <88.88%> (-0.03%) ⬇️
unit 35.31% <66.66%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
client/web/src/integration/jscontext.ts 0.00% <ø> (ø)
cmd/frontend/internal/app/jscontext/jscontext.go 1.44% <0.00%> (-0.03%) ⬇️
internal/conf/computed.go 21.64% <0.00%> (-0.50%) ⬇️
client/web/src/user/settings/routes.tsx 44.44% <50.00%> (+0.44%) ⬆️
...web/src/user/settings/research/ProductResearch.tsx 100.00% <100.00%> (ø)
client/web/src/user/settings/sidebaritems.ts 100.00% <100.00%> (ø)
...t/web/src/search/input/toggles/CopyQueryButton.tsx 14.28% <0.00%> (-57.15%) ⬇️
internal/search/query/alert.go 38.23% <0.00%> (-11.77%) ⬇️
...ion/after-install-page/AfterInstallPageContent.tsx 83.33% <0.00%> (-6.67%) ⬇️
client/shared/src/components/icons.tsx 63.63% <0.00%> (-4.55%) ⬇️
... and 45 more

@umpox umpox marked this pull request as ready for review February 5, 2021 13:49
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Feb 5, 2021

Notifying subscribers in CODENOTIFY files for diff 32dda4a...f3cc0b0.

Notify File(s)
@keegancsmith internal/conf/computed.go

@umpox umpox requested review from a team and keegancsmith February 5, 2021 13:51
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Code changes LGTM 👍

Copy link
Contributor

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

LGTM otherwise! Awesome that you took action to improve the tests 💯

@umpox umpox requested a review from quinnkeast February 8, 2021 11:01
@umpox umpox merged commit f2c9dd5 into main Feb 10, 2021
@umpox umpox deleted the tr/product-feedback-page branch February 10, 2021 09:44
authenticatedUser: Pick<AuthenticatedUser, 'email'>
}

const signUpForm = new URL('https://share.hsforms.com/1tkScUc65Tm-Yu98zUZcLGw1n7ku')
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed this: This feels a bit weird because this URL is mutated on every render but shared between all instances of a component. This could cause issues when rendering it multiple times, e.g. in tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't cause issues as set will just override any previous values but I agree this is a bit weird, PR to fix: https://github.com/sourcegraph/sourcegraph/pull/18171

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants