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

Manage Students Table: UI for cog icon drop down in action header cell #21107

Merged
merged 7 commits into from Mar 9, 2018

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Mar 8, 2018

This is the initial UI work for the dropdown that appears when the cog icon in the header of the action cell in the Manage Students table is clicked.
cog-click

  • Passed a type prop to QuickActionsCell such that it can be extended to use the cog icon
  • Extended Stories to reflect the additional prop
  • Wrapped QuickActionsCell in Radium so it can be styled conditionally based on type
  • Made a new ManageStudentsActionsHeaderCell that will be further customized to add the functionality for the actions in the dropdown in follow up PRs

@Erin007 Erin007 requested a review from caleybrock March 8, 2018 21:09
@caleybrock
Copy link
Contributor

Nit: Looks like some margin is added to the icon when the dialog is opened, so it looks like it moves. It'd be best it if stayed in place.

@Erin007
Copy link
Contributor Author

Erin007 commented Mar 8, 2018

🤔 @caleybrock The styling of the cog icon is the same whether the dialog is open or closed. Maybe you caught this before the latest commit?

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

A few small nits, but looking good overall!

@@ -7,8 +7,8 @@ export default storybook => {
.storiesOf('QuickActionsCell', module)
.addStoryTable([
{
name: 'QuickActionsCell',
description: 'Shown with 2 QuickActions as children',
name: 'QuickActionsCell - default body',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice additions to storybook here.

@@ -105,4 +140,4 @@ class QuickActionsCell extends Component {
}
}

export default QuickActionsCell;
export default Radium(QuickActionsCell);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for Radium now right?

};

static defaultProps = {
type: 'body'
Copy link
Contributor

Choose a reason for hiding this comment

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

replace with QuickActionsCellType.body

body: 'chevron-down'
};

const styleByType = type === "header" ?
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above. replace strings with QuickActionsCellType.type

},
[QuickActionsCellType.header]: {
fontSize: 20,
marginTop: -3,
Copy link
Contributor

Choose a reason for hiding this comment

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

Negative margins are not the end of the world, but can be kind of hard to be debug. Was this the only way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was the easiest way I could think of to do it. The icon has a bigger font size than the text and I wanted it to look more or less vertically aligned. Is there a better way?

@caleybrock
Copy link
Contributor

Hmm yeah it doesn't look like it's moving now.

Copy link
Contributor

@caleybrock caleybrock left a comment

Choose a reason for hiding this comment

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

One more change and then I think this looks good.

This was a really good use case for the cell type enum. Good call there 😄

@@ -0,0 +1,29 @@
import React, {Component} from 'react';
import QuickActionsCell from "../tables/QuickActionsCell";
Copy link
Contributor

Choose a reason for hiding this comment

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

If you change this line to:
import QuickActionsCell, {QuickActionsCellType} from "../tables/QuickActionsCell";
Then on line 12, you can use:
type={QuickActionsCellType.header}

@caleybrock
Copy link
Contributor

Would also be good to add a test for what shows up in the dropdown, since the dialog will change in the future.
I think you can basically copy the test from here:
https://github.com/code-dot-org/code-dot-org/blob/staging/apps/test/unit/templates/manageStudents/ManageStudentsActionsCellTest.js#L15

Totally fine to make in a separate PR

@Erin007 Erin007 merged commit d0b63dd into staging Mar 9, 2018
@Erin007 Erin007 deleted the cog-click-quick-actions branch March 9, 2018 18:49
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

2 participants