Skip to content

Add variant analysis top header#1512

Merged
koesie10 merged 12 commits intomainfrom
koesie10/variant-analysis-header
Sep 19, 2022
Merged

Add variant analysis top header#1512
koesie10 merged 12 commits intomainfrom
koesie10/variant-analysis-header

Conversation

@koesie10
Copy link
Copy Markdown
Member

This adds the variant analysis top header. It also adds React testing library to make automated testing of this possible and adds stories for the newly added component.

It might be easier to review this commit-by-commit.

Since this is only part of header, we need a better name for this component. However, I couldn't really think of one. The best one I could come up with is VariantAnalysisTitleRow but that doesn't really cover everything.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 requested review from a team as code owners September 14, 2022 14:19
Copy link
Copy Markdown
Contributor

@charisk charisk 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! A partial review from me atm.

@koesie10 koesie10 requested a review from a team September 15, 2022 08:18
@@ -1,9 +1,13 @@
import styled from 'styled-components';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it worth moving this file into a shared location (view/common rather than having some stuff in remote-queries and some in variant-analysis)? I'm happy for this to be in a subsequent PR if we agree it's the right thing to do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍, I think that would be very useful. We can also consider making these importable by just specifying the ../common when we place an index.ts file inside the common directory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great! Are you happy to do that as part of the issue you're working on, as a separate PR?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It might be better to do that in a separate PR. There are some other components which we might also be able to move to the common directory such as VerticalSpace, so the changeset might get quite large (especially since all imports also need to be changed).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I'd definitely prefer to see it in a separate PR!

@koesie10 koesie10 requested a review from a team September 16, 2022 10:10
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Really great work! Thanks @koesie10!

@koesie10 koesie10 merged commit 4b8d611 into main Sep 19, 2022
@koesie10 koesie10 deleted the koesie10/variant-analysis-header branch September 19, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants