Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Feature/161418919 implement new search result card #8

Closed

Conversation

lingyun1010
Copy link
Contributor

This work is based on a new atlas branch, which provides the full JSON payload with number of assays and experimental variables, which are rendered in the new gene search result cards.

Copy link
Member

@alfonsomunozpomer alfonsomunozpomer left a comment

Choose a reason for hiding this comment

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

There are quite a few observations about this component.

The first one is that as far as I can see, unless https://www.pivotaltracker.com/story/show/161418753 is merged it won’t really work. That’s why both this branch and the aforementioned issue should go together (and that’s why tasks shouldn’t be ultimately split unless we approve so by explicitly discussing it). If both this new ExperimentCard component and the endpoint that provides number of assays and experimental variables were packed in the same branch, we would have a nice, clean, piece of code that would cover all the functionality required by the story.

The second issue is that this new component replaces the ExperimentCard found in sc/src/javascript/bundles/gene-search/src/ExperimentCard.js. There’s no reason why you should know that, though, and you’ll get more acquainted with the codebase with time.

Also, we don’t use CSS files to style components. Use styled-components since it achieves the same level of fine-grained scoping without the complexity of CSS Modules. We’ve found through experience that packing CSS files is very problematic.

Lastly, whenever you apply styles, never use IDs. Use classes. Be aware that more than one card will be rendered in the search results, which means that if you use IDs more than one component will have the same ID, and that’s not valid HTML.

Please address these issues in a branch in the Atlas repository. Make it part of ebi-gene-expression-group/atlas#188 to make testing and reviewing this issue easier for the rest of us.

The card itself looks good, though! 😅

@lingyun1010 lingyun1010 deleted the feature/161418919-implement-new-search-result-card branch November 6, 2018 15:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants