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

create GlobalLinterMessages component #872 #893

Merged

Conversation

mirefly
Copy link
Collaborator

@mirefly mirefly commented Jun 28, 2019

Fixes #872
@kumar303
In refactoring, I found scrolling to linter messages in different files is not work well (Not sure why..) #887 (comment).
I tested some pages, this refactoring does not change behaviors when navigating between linter messages.

@codecov-io
Copy link

codecov-io commented Jun 28, 2019

Codecov Report

Merging #893 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #893   +/-   ##
======================================
  Coverage    98.8%   98.8%           
======================================
  Files          53      54    +1     
  Lines        1673    1673           
  Branches      402     402           
======================================
  Hits         1653    1653           
  Misses         19      19           
  Partials        1       1
Impacted Files Coverage Δ
src/components/DiffView/index.tsx 100% <100%> (ø) ⬆️
src/components/CodeView/index.tsx 100% <100%> (ø) ⬆️
src/components/GlobalLinterMessages/index.tsx 100% <100%> (ø)

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 9d98db1...95993e4. Read the comment docs.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This is looking great so far, thanks for getting this started. I have a few change requests but nothing major. Let me know if you have questions.

src/components/GlobalLinterMessages/index.tsx Outdated Show resolved Hide resolved
src/components/CodeView/index.tsx Outdated Show resolved Hide resolved
src/components/CodeView/index.tsx Outdated Show resolved Hide resolved
src/components/GlobalLinterMessages/index.tsx Show resolved Hide resolved
src/components/GlobalLinterMessages/index.tsx Outdated Show resolved Hide resolved
src/components/GlobalLinterMessages/index.tsx Outdated Show resolved Hide resolved
@mirefly mirefly force-pushed the create-GlobalLinterMessages-component-I872 branch from 405047e to 2fe6782 Compare June 29, 2019 20:01
@mirefly
Copy link
Collaborator Author

mirefly commented Jun 29, 2019

@kumar303 I believe I addressed all comments.
I have one question about the type of containerRef. Now I used the type of scrollToSelectedLine in CodeView, but not sure why to use HTMLTableRowElement instead of HTMLElement?

export const scrollToSelectedLine = (element: HTMLTableRowElement | null) => {

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review. I answered your ref question and had a few other change requests, mostly minor cleanup.

src/components/CodeView/index.spec.tsx Show resolved Hide resolved
src/components/CodeView/index.spec.tsx Show resolved Hide resolved
src/components/GlobalLinterMessages/index.tsx Outdated Show resolved Hide resolved
src/components/GlobalLinterMessages/index.tsx Outdated Show resolved Hide resolved
src/components/GlobalLinterMessages/index.spec.tsx Outdated Show resolved Hide resolved
src/components/GlobalLinterMessages/index.spec.tsx Outdated Show resolved Hide resolved
src/components/GlobalLinterMessages/index.spec.tsx Outdated Show resolved Hide resolved
@mirefly mirefly force-pushed the create-GlobalLinterMessages-component-I872 branch from 2fe6782 to 8aebe07 Compare July 2, 2019 22:49
@mirefly
Copy link
Collaborator Author

mirefly commented Jul 2, 2019

Thanks for reviewing @kumar303 . I updated and rebased it.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This is looking very good but I noticed a couple small issues.

src/components/CodeView/index.tsx Show resolved Hide resolved
src/components/GlobalLinterMessages/index.tsx Outdated Show resolved Hide resolved
@kumar303
Copy link
Contributor

kumar303 commented Jul 3, 2019

The snyk failure has been fixed on master so you can rebase again to get the fix (sorry about all the rebasing!)

@mirefly mirefly force-pushed the create-GlobalLinterMessages-component-I872 branch from 8aebe07 to 95993e4 Compare July 3, 2019 16:33
@mirefly
Copy link
Collaborator Author

mirefly commented Jul 3, 2019

Updated! :) @kumar303

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Thanks for all the quick follow-ups!

@kumar303 kumar303 merged commit a88cd5d into mozilla:master Jul 3, 2019
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.

Add a shared GlobalLinterMessages component
3 participants