Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

Feature/162416792 create new experiment table #1

Merged
merged 66 commits into from
May 8, 2019

Conversation

lingyun1010
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@monicajianu monicajianu left a comment

Choose a reason for hiding this comment

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

There is a lot going on in the ExperimentTable.js file. I think it could be broken down into a few separate components.

Another thing is variable naming. A lot more could be done to make them clearer and more descriptive.

And the biggest thing is some issues around styling. The browse experiments page in the web app uses the row expanded (i.e. wide) class. If you use this in your #wrapper element in the index.html file, you will achieve the same look. And as it is, that look is a little bit empty. The columns should take up all the space available...

Screenshot of the wide format

If I remember correctly, this is a limitation of Evergreen, which is why I wasn't a big fan of this table in the first place. But see if you can get it to fill the space, rather than stick to pre-defined widths.

Another thing is that the header is not very responsive. If you shrink the browser too much, you start losing header cells...:

Missing header cells

src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
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.

I think I rushed a bit the review comments: what we want is to remove completely any remote fetch logic from the package. The demo pages should pass the data directly to the component, and it would be our choice, when we add a wrapper to the bundles dir, whether to use atlas-react-fetch-loader or any other mechanism as we see fit in order to feed data to the table.

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’s a lot of HTML to clean up here. I’ve written down what I’ve spotted to on a first pass. Once you’ve done so, please let me know, and take your time to read the Foundation (float) grid layout, which is what we’re using in the EBI Framework.

The logic though seems solid and tests look fine, too.

src/TableFooter.js Outdated Show resolved Hide resolved
src/TableFooter.js Outdated Show resolved Hide resolved
src/TableFooter.js Outdated Show resolved Hide resolved
src/TableFooter.js Outdated Show resolved Hide resolved
src/TableFooter.js Outdated Show resolved Hide resolved
src/TableFooter.js Outdated Show resolved Hide resolved
src/tableHeaderCells.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
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.

Good second iteration.

Besides the changes suggested, I think it’d be better if ExperimentTable is further broken down between top search boxes, the table body itself and the table footer (that one has already been refactored). In that way you’d have a kind of top-level component with the basic layout, and the other three.

src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
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.

Good work! Another iteration. I focused on reviewing mostly ExperimentTable.js and it’s now getting in shape. :)

One thing you should try to care about is to break long lines in logical blocks and add blank lines to improve readability.

src/index.js Outdated Show resolved Hide resolved
src/TableContent.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Show resolved Hide resolved
src/TableContent.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Show resolved Hide resolved
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.

Good stuff! We’re 95% done!

Besides the comments you should also add what the shape or types of the arrays you will receive in props using PropTypes.arrayOf instead of PropTypes.array.

src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/ExperimentTable.js Outdated Show resolved Hide resolved
src/TableSearchHeader.js Outdated Show resolved Hide resolved
src/TableSearchHeader.js Outdated Show resolved Hide resolved
src/TableContent.js Outdated Show resolved Hide resolved
src/TableContent.js Show resolved Hide resolved
src/TableContent.js Outdated Show resolved Hide resolved
src/tableHeaderCells.js Outdated Show resolved Hide resolved
src/TableFooter.js Show resolved Hide resolved
@lingyun1010 lingyun1010 merged commit cb4b0c3 into master May 8, 2019
@alfonsomunozpomer alfonsomunozpomer deleted the feature/162416792-create-new-experiment-table branch May 8, 2019 13:46
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

3 participants