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.

Sign up for GitHub

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

Jump to bottom

refactor: Header component #257

Merged
merged 16 commits into from
Mar 13, 2024
Merged

refactor: Header component #257

merged 16 commits into from
Mar 13, 2024

Conversation

ShrimpCryptid
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid commented Mar 8, 2024 •
edited
Loading

Problem

Refactors the header into a reusable series of components as part of #250.

Estimated review size: small, <15 minutes. (Recommend turning off whitespace)

Solution

  • Moves the header bar into its own Header.tsx component file, which pages can reuse.
    • We can't mount the header as part of the root component, because many of the buttons that go in the header (export, copy, etc.) are context-specific and require access to page state.
    • This component will eventually be used by the landing page.

Type of change

  • New feature (non-breaking change which adds functionality)

Steps to Verify:

  1. Open the preview link and confirm that the inputs and interactions in the header work: https://allen-cell-animated.github.io/nucmorph-colorizer/pr-preview/pr-257/
  2. Open the error page. The header should also appear: https://allen-cell-animated.github.io/nucmorph-colorizer/pr-preview/pr-257/#/bad

Screenshots (optional):

image

@ShrimpCryptid ShrimpCryptid added the internals Tech debt, refactoring, dependencies, etc. label Mar 8, 2024
@ShrimpCryptid ShrimpCryptid self-assigned this Mar 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024 •
edited
Loading

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-03-13 22:47 UTC

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 8, 2024 •
edited
Loading

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 65.56% 4033 / 6151
🔵 Statements 65.56% 4033 / 6151
🔵 Functions 52.48% 95 / 181
🔵 Branches 80.05% 313 / 391
File CoverageNo changed files found.
Generated in workflow #745

@ShrimpCryptid ShrimpCryptid marked this pull request as ready for review March 8, 2024 19:58
@ShrimpCryptid ShrimpCryptid requested a review from a team as a code owner March 8, 2024 19:58
@ShrimpCryptid ShrimpCryptid requested review from meganrm and frasercl and removed request for a team March 8, 2024 19:58
Base automatically changed from refactor/app-router to main March 8, 2024 19:59
@ShrimpCryptid ShrimpCryptid marked this pull request as draft March 8, 2024 20:00
ShrimpCryptid
ShrimpCryptid commented Mar 8, 2024
View reviewed changes
src/assets/AICS-logo-and-name.svg Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edited the SVG's title attribute to say "Allen Institute for Cell Science"

ShrimpCryptid
ShrimpCryptid commented Mar 8, 2024
View reviewed changes
src/Viewer.module.css Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to Header.tsx

@ShrimpCryptid ShrimpCryptid marked this pull request as ready for review March 8, 2024 20:13
@meganrm
Copy link
Contributor

meganrm commented Mar 11, 2024

the preview doesn't load the dataset (I get an error) is that expected?

@meganrm
Copy link
Contributor

meganrm commented Mar 11, 2024

Design feedback: it seems weird to have the help be a dropdown that only has one item. is that because you plan to have more in that dropdown in the future?

@ShrimpCryptid
Copy link
Contributor Author

the preview doesn't load the dataset (I get an error) is that expected?

It should be loading... Are you on VPN?

@ShrimpCryptid
Copy link
Contributor Author

Design feedback: it seems weird to have the help be a dropdown that only has one item. is that because you plan to have more in that dropdown in the future?

Yup! There are a few issues in the backlog for adding additional options to the help dropdown. See #241 and #239.

frasercl
frasercl reviewed Mar 13, 2024
View reviewed changes
src/components/Header/index.tsx Outdated
import Header from "./Header";
import HeaderLogo from "./HeaderLogo";

export { Header, HeaderLogo };
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be easier to just put the logo inside the header in the first place? I don't think there's a scenario where Header would be used and HeaderLogo wouldn't be the immediate first child. Then this module could be one file instead of three (for now).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was worried that this wouldn't work but was pleasantly surprised when it did. Thank you for the suggestion!

frasercl
frasercl approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@frasercl frasercl 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!

@ShrimpCryptid ShrimpCryptid merged commit 9e7a337 into main Mar 13, 2024
2 checks passed
@ShrimpCryptid ShrimpCryptid deleted the refactor/header-component branch March 13, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meganrm meganrm meganrm approved these changes

@frasercl frasercl frasercl approved these changes

Assignees

@ShrimpCryptid ShrimpCryptid

Labels
internals Tech debt, refactoring, dependencies, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ShrimpCryptid @meganrm @frasercl