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

Collapsible Side Panel #105

Merged

Conversation

apiraam96
Copy link
Collaborator

Screenshot 2024-02-21 at 12 56 05 PM

Closes Issue #86

@zkdan zkdan self-requested a review February 22, 2024 22:04
Copy link
Collaborator

@zkdan zkdan left a comment

Choose a reason for hiding this comment

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

A good start! The moon icon is a nice touch.

I believe the takeaways from the meeting on 2/22/24 were:

  • The sidebar content being invisible when the sidebar is collapsed
  • The icon to slide the sidebar in and out should be larger and visually in its own container like a tab on a file
  • The logo should always be visible

@apiraam96 apiraam96 requested a review from zkdan March 14, 2024 21:08
Copy link
Collaborator

@zkdan zkdan left a comment

Choose a reason for hiding this comment

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

You're not using the react-pro-sidebar, right? I bet you just need to delete your package-lock file so that one can be regenerated that will have no conflicts with staging. If you're having trouble, you may want to totally delete node_modules and regenerate everything from staging's package file.

It looks great though!

@apiraam96 apiraam96 requested a review from zkdan March 21, 2024 03:22
@apiraam96 apiraam96 requested review from mwkyuen and Yukthiw March 27, 2024 15:28
Copy link
Collaborator

@Yukthiw Yukthiw left a comment

Choose a reason for hiding this comment

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

Looks great, just a note on how to use Jotai consistent with the rest of the project.

@apiraam96 apiraam96 requested a review from Yukthiw March 28, 2024 19:28
Copy link
Collaborator

@Yukthiw Yukthiw left a comment

Choose a reason for hiding this comment

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

LGTM

@zkdan zkdan removed the request for review from mwkyuen March 28, 2024 20:47
@apiraam96
Copy link
Collaborator Author

@asherpasha

@asherpasha
Copy link
Contributor

asherpasha commented Mar 29, 2024

I think workflows need to be updated to run GitHub checks on pull request. Currently, it's only on push.

See BAR API example:
https://github.com/BioAnalyticResource/BAR_API/blob/dev/.github/workflows/bar-api.yml

@Yukthiw
Copy link
Collaborator

Yukthiw commented Mar 29, 2024

I think workflows need to be updated to run GitHub checks on pull request. Currently, it's only on push.

See BAR API example: https://github.com/BioAnalyticResource/BAR_API/blob/dev/.github/workflows/bar-api.yml

I think there's a setting change that needs to be done, having a PR trigger just runs the action when you first open a PR which is not what we want. See this StackOverflow post @asherpasha

@asherpasha
Copy link
Contributor

This is done. Please try again.

@apiraam96
Copy link
Collaborator Author

apiraam96 commented Mar 30, 2024

I'm still having the pending problem with the build and lint-and-format checks. It looks like there are other PRs from Jamie and Mike that want to merge their forked branches to staging too

@apiraam96 apiraam96 merged commit 058e273 into BioAnalyticResource:staging Mar 30, 2024
2 checks passed
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.

4 participants