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

add button to redirect to results page #129

Merged

Conversation

eng-esther
Copy link
Contributor

remove absolute
Need to discuss with team first before moving the button above the search input field

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #129 (eb4309b) into master (1ab1f09) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #129   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines          325       346   +21     
  Branches        35        41    +6     
=========================================
+ Hits           325       346   +21     
Impacted Files Coverage Δ
src/theme/components.js 100.00% <ø> (ø)
src/components/Search/SearchView.tsx 100.00% <100.00%> (ø)
src/tests/utils/test-utils.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 179a20d...eb4309b. Read the comment docs.

Copy link
Contributor

@kimberlythegeek kimberlythegeek left a comment

Choose a reason for hiding this comment

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

looks good--thanks!

@julienw
Copy link
Contributor

julienw commented Jun 1, 2022

Hey @eng-esther ! Can you please clean up the commit log? currently it contains the message in your former latest commit, so it doesn't reflect the content of this patch.

Also I see there are a lot of conflicting files, it would be good to rebase the patch before we can review. Most of them are snaphot files so these ones should be as easy as running the test runner with -u.

Thanks!

@julienw
Copy link
Contributor

julienw commented Jun 1, 2022

I also see lint errors in this patch when I try to run it locally, can you please take care to fix them? Thanks so much :-)

update: I don't see them when running the linter, only when running the dev enviromnent:
image

So not sure what's happening here... Maybe you can ignore my comment for now.

Copy link
Contributor

@kimberlythegeek kimberlythegeek left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@eng-esther eng-esther force-pushed the add-button-to-go-to-results-page branch from 0ad3484 to eb4309b Compare June 3, 2022 12:05
@eng-esther eng-esther requested a review from julienw June 3, 2022 14:01
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

lgtm!

@eng-esther eng-esther merged commit b87cfbb into mozilla:master Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants