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 experiment page bundle #490

Merged
merged 10 commits into from
Dec 9, 2024
Merged

Refactor experiment page bundle #490

merged 10 commits into from
Dec 9, 2024

Conversation

lingyun1010
Copy link
Contributor

We are going to simplify the logic and deployment of experiment page router component and trying to move the bundle to the front end component repository. The package has not been officially published yet, so in this PR, I only installed a local compiled package, you may test the refactoring results by doing the same.

@lingyun1010 lingyun1010 added improvement Improve/refactor existing code experiment page labels Nov 12, 2024
@lingyun1010 lingyun1010 self-assigned this Nov 12, 2024
@lingyun1010 lingyun1010 linked an issue Nov 13, 2024 that may be closed by this pull request
[Chore] Experiment page bundle refactoring ebi-gene-expression-group/atlas-components#171
Closed
3 tasks
ke4
ke4 requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 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 your work.
I added just 2 comments to double check my understanding of the related code in the scxa-experiment-page package.

upendrakumbham
upendrakumbham requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@upendrakumbham upendrakumbham left a comment

Choose a reason for hiding this comment

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

Please add validations where you need before fetching data and consider isRequired props validations mandatory please.

@lingyun1010
Copy link
Contributor Author

Please add validations where you need before fetching data and consider isRequired props validations mandatory please.

I think that's not this PR about. That is one of your previous work isn't it? The validation?

ke4
ke4 requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment

Choose a reason for hiding this comment

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

Hi,
I am still not sure what is the anatomogram property in the ExperimentPageRouter.
I added a longer comment regarding to that one.
Could you please clarify it?
Thanks

app/src/main/javascript/bundles/experiment-page/index.js Outdated Show resolved Hide resolved
app/src/main/javascript/bundles/experiment-page/package.json Outdated Show resolved Hide resolved
@lingyun1010
Copy link
Contributor Author

Hi, I am still not sure what is the anatomogram property in the ExperimentPageRouter. I added a longer comment regarding to that one. Could you please clarify it? Thanks

Yes, I think you are right, we have anatomogram props under variable tabs, i.e., anatomogram is an entry of tabs, but here is a redundant or bug line, as we don't have anatomogram props under content. I removed this line, please take a further look.

@lingyun1010 lingyun1010 requested a review from ke4 December 5, 2024 10:03
ke4
ke4 previously approved these changes Dec 5, 2024
View reviewed changes
Copy link
Contributor

@ke4 ke4 left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for clarifying my suggestion.

upendrakumbham
upendrakumbham previously approved these changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@upendrakumbham upendrakumbham left a comment

Choose a reason for hiding this comment

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

LGTM!

@lingyun1010 lingyun1010 merged commit 89b4508 into develop Dec 9, 2024
3 checks passed
@lingyun1010 lingyun1010 deleted the chore/refactor-experiment-page-bundle branch December 9, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upendrakumbham upendrakumbham upendrakumbham left review comments

@ke4 ke4 ke4 left review comments

Assignees

@lingyun1010 lingyun1010

Labels
chore experiment page improvement Improve/refactor existing code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Chore] Experiment page bundle refactoring
3 participants
@lingyun1010 @ke4 @upendrakumbham