Skip to content
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

Hide the search results banner if a repo: field is present on sg.com #5142

Merged
merged 3 commits into from Aug 9, 2019

Conversation

ijt
Copy link
Contributor

@ijt ijt commented Aug 9, 2019

Test plan: manually tested by commenting out && props.showDotComMarketing locally

@codecov
Copy link

codecov bot commented Aug 9, 2019

Codecov Report

Merging #5142 into master will increase coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #5142      +/-   ##
==========================================
+ Coverage   46.22%   46.22%   +<.01%     
==========================================
  Files         734      734              
  Lines       44992    44994       +2     
  Branches     2602     2604       +2     
==========================================
+ Hits        20796    20797       +1     
- Misses      22186    22187       +1     
  Partials     2010     2010
Impacted Files Coverage Δ
web/src/search/results/SearchResultsInfoBar.tsx 67.74% <100%> (ø) ⬆️
web/src/search/results/SearchResultsList.tsx 60.95% <50%> (-0.16%) ⬇️

@ijt ijt merged commit 5c3d293 into master Aug 9, 2019
@ijt ijt deleted the banner branch August 9, 2019 15:57
@ijt
Copy link
Contributor Author

ijt commented Aug 9, 2019

It somehow does not appear to work on sourcegraph.com:

Screen Shot 2019-08-09 at 9 14 10 AM

Copy link
Contributor

@nicksnyder nicksnyder left a comment

Choose a reason for hiding this comment

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

It worked locally and not on sourcegraph.com? That is weird. Are you sure this change got deployed?

@@ -149,7 +152,7 @@ export const SearchResultsInfoBar: React.FunctionComponent<SearchResultsInfoBarP
</div>
</small>
)}
{!props.results.alert && props.showDotComMarketing && <ServerBanner />}
{!props.results.alert && props.showDotComMarketing && !props.hasRepoishField && <ServerBanner />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I didn't review this carefully enough (I was on my phone, sorry). I think it would be better to not hide the banner, but instead change the message (to what it was before) if there is a repo: or repogroup: param (as I described in #5097 (comment)).

@ijt
Copy link
Contributor Author

ijt commented Aug 9, 2019

It worked locally and not on sourcegraph.com? That is weird. Are you sure this change got deployed?

@ggilmore, do you know if this was deployed?

@ggilmore
Copy link
Contributor

ggilmore commented Aug 9, 2019

@ijt Yes, master is already running 763791c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants