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

Re-factor: Reacttabular styles and table actions #18526

Merged
merged 6 commits into from Oct 20, 2017

Conversation

caleybrock
Copy link
Contributor

@caleybrock caleybrock commented Oct 19, 2017

This PR does:

  • create a common set of styles for components using react-tabular
  • make ManageStudentsTable, PersonalProjectsTable, and SectionTable use the common styles
  • moves QuickActions to a common table location so other tables that need actions can use it (todo: add to ManageStudentsTable)

This PR does NOT:

  • touch SectionsTable (not to be confused with SectionTable), which does not use react-tabular, but still has a set of table styles

What's next?

  • [owner: me] continue to make QuickActions more re-usable by also creating a common child component
  • [owner: unknown] convert SectionsTable to use react-tabular/tableConstants to keep it consistent with the rest of our tables
  • [owner: poorva to decide if we should] update ProjectList with the new styles, since it already uses react-tabular, but has angular styles. Mark said to hold off on this one until getting specific instruction, but this would be a very simple work item now

Screenshots:
ManageStudentsTable
screen shot 2017-10-19 at 3 57 07 pm

PersonalProjectsTable
screen shot 2017-10-19 at 3 57 24 pm

SectionTable
screen shot 2017-10-19 at 3 53 09 pm

@caleybrock
Copy link
Contributor Author

cc werketing @breville @tanyaparker @Erin007

@caleybrock caleybrock requested review from islemaster, Erin007 and Bjvanminnen and removed request for islemaster October 19, 2017 23:07
@caleybrock
Copy link
Contributor Author

caleybrock commented Oct 19, 2017

Switching Brad out for Brent as reviewer since Brad has a presentation tomorrow.

@Erin007
Copy link
Contributor

Erin007 commented Oct 19, 2017

What happens when any of these tables are RTL?

import color from "../../util/color";
import styleConstants from "../../styleConstants";

// Constants for React tables
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this entire file is generally stuff for reacttabular tables. I wonder if we should rename it to reflect that, i.e. reactabularStyles.js or reactabularConstants.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I didn't do this, I think html tables could be setup to work with these styles, so I purposefully kept it general.

Copy link
Contributor

@Bjvanminnen Bjvanminnen left a comment

Choose a reason for hiding this comment

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

Nice job simplifying these

Copy link
Contributor

@Erin007 Erin007 left a comment

Choose a reason for hiding this comment

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

Hooray! This is so much better!

@caleybrock
Copy link
Contributor Author

@Erin007 Good question. The styles on these are all the same as before, so RTL is the same as it currently looks. That being said I imagine there are some improvements we could make there, because it doesn't look like any of them have specific support.

@caleybrock caleybrock merged commit b359bdf into staging Oct 20, 2017
@caleybrock caleybrock deleted the common-table-refactor branch October 20, 2017 00:29
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.

None yet

3 participants