Section Action Dropdown #18058

audreyclark · 2017-09-29T23:05:36Z

This section table action dropdown removes buttons from the section table and replaces with an expandable list of actions.

@caleybrock is planning to refactor this functionality as she works on react-ing other tables.

[Before]

[After - Google Classroom section]

[After - deletable section]

[Confirm Delete Dialog]

…it tests.

islemaster · 2017-10-18T19:04:04Z

❤️ This looks so much better!

islemaster · 2017-10-18T19:06:13Z

I have two visual nits with this dialog:

There's a lot of extra whitespace at the top. Can we even out the margins?
Both buttons shouldn't be orange call-to-action buttons. "Cancel" should probably be gray/secondary. Orange/CTA is probably okay for "Delete" but I wonder if Red/danger would be better? Since it's an irreversible action.

islemaster

This is great and I love that we get to tear out a bunch of code as part of this change. The new dropdown is a pretty big component - I have some comments there and generally wonder if there are other ways we can break it up a bit.

islemaster · 2017-10-18T19:16:00Z

apps/src/templates/teacherDashboard/SectionActionBox.jsx

+                {i18n.deleteSection()}
+              </div>
+            </a>
+            <BaseDialog


Even if you leave it at the end of this file, it'd help readability to extract this delete confirmation dialog as its own component. Fortunately there's very little in the way of props you'd need to pass through.

islemaster · 2017-10-18T19:19:43Z

apps/src/templates/teacherDashboard/SectionActionBox.jsx

+              style={{padding:20}}
+            >
+              <h2>{i18n.deleteSection()}</h2>
+              <div>{i18n.deleteSectionConfirm()}</div>


This dialog should include the section name somewhere in its title or description text - otherwise we're depending on the user remembering which section they picked before the dialog opened, which isn't obvious after the fact.

Also note that we're making a dialog for every section row. For users with a ton of sections it may work better to render the dialog once at the page level and show/hide it based on redux state. After all, you shouldn't be able to show more than one delete dialog at a time.

islemaster · 2017-10-18T19:20:30Z

apps/src/templates/teacherDashboard/SectionActionBox.jsx

+  };
+
+  onConfirmDelete = () => {
+      const {removeSection } = this.props;


Nit: Unbalanced whitespace.

islemaster · 2017-10-18T19:23:21Z

apps/src/templates/teacherDashboard/SectionActionBox.jsx

+        // While we are embedded in an angular page, reloading is the easiest
+        // way to pick up roster changes.  Once everything is React maybe we
+        // won't need to do this.
+        utils.reload();


Probably should be considered out-of-scope of this PR, but it'd be nice to address this comment and allow syncing without a page reload. Might also want some kind of progress or loading spinner intermediate state.

islemaster · 2017-10-18T19:25:34Z

apps/src/templates/teacherDashboard/SectionActionBox.jsx

+  onConfirmDelete = () => {
+      const {removeSection } = this.props;
+      const section = this.props.sectionData;
+      $.ajax({


We should handle the intermediate state while this request is in flight gracefully - at least disable the confirm delete button so we can't request this twice, possibly hide the dialog immediately but put the row in a 'pending' state until it gets removed, that sort of thing.

islemaster · 2017-10-18T19:29:47Z

dashboard/test/ui/features/SectionActionDropdown.feature

@@ -0,0 +1,57 @@
+@no_mobile
+@dashboard_db_access
+@pegasus_db_access


Do we actually need both of these tags?

islemaster · 2017-10-18T19:32:06Z

dashboard/test/ui/features/SectionActionDropdown.feature

+@no_mobile
+@dashboard_db_access
+@pegasus_db_access
+Feature: Using the SectionActionDropdown


Great new tests! I suspect we had some UI tests elsewhere that depended on the buttons we're removing with this change. Have you checked for this?

islemaster · 2017-10-18T19:34:04Z

Also I'll pull this and give it a try.

islemaster · 2017-10-18T20:21:32Z

I haven't tracked down a root cause, but dismissing the dropdown feels sluggish. The delay between deselecting the menu and having it vanish isn't great.

islemaster · 2017-10-18T20:26:09Z

I also get different cursors for menu items that are implemented as hyperlinks and those that are implemented with onClick handlers. We should also make this text unselectable to avoid the text selection cursor and accidental selection instead of clicking on a menu item. It be nice to unify this.

Underline for the current menu item is also a little weird - I think convention for a context menu like this is usually to highlight the currently selected row. IMO that might look better. Obviously defer to Tanya on this.

islemaster · 2017-10-18T20:27:27Z

apps/src/templates/teacherDashboard/SectionActionDropdown.jsx

+import SectionActionBox from "./SectionActionBox";
+
+const styles = {
+  actionButton:{


Please add cursor:pointer to the chevron button. and maybe a hover style on the button; it doesn't feel super discoverable right now.

… trigger

epeach · 2017-11-16T01:17:59Z

@islemaster I think this is ready for another code review, when you get the chance. Thanks!

caleybrock · 2017-11-16T01:27:30Z

apps/src/lib/ui/PopUpMenu.jsx

@@ -116,46 +120,84 @@ class MenuBubbleUnwrapped extends Component {
      <div style={style} className={className}>
        {this.renderMenuItems()}
        {/* These elements are used to draw the 'tail' with CSS */}
-        <span style={tailBorderStyle}/>
-        <span style={tailFillStyle}/>
+        {!this.props.minimalStyle &&


I think it'd be nice to have Mark/Poorva choose one style for these. As discussed, the carrot on/off is a reasonable flag, but it feels like we would want these to be the same anyways.

caleybrock · 2017-11-16T01:28:16Z

apps/src/lib/ui/PopUpMenu.jsx

    first: PropTypes.bool,
    last: PropTypes.bool,
+    color: PropTypes.string,
+    heightDivisor: PropTypes.number,


This part in particular would be great to not have.

Reading this props list without the component implementation, I don't really know what heightDivisor does. At least having a comment documenting this prop would be nice, so someone wanting to reuse Item wouldn't need to read its whole implementation.

caleybrock · 2017-11-16T18:30:07Z

apps/src/lib/ui/PopUpMenu.jsx

    };
+
+    const Tag = href ? 'a' : 'div';


I did not know you could do this. It feels a bit weird to have a href="null" for a div tag, but apparently this is what we do with Button.

caleybrock · 2017-11-16T18:30:47Z

apps/src/lib/ui/PopUpMenu.story.jsx

-          <PopUpMenu.Item>Option One</PopUpMenu.Item>
-          <PopUpMenu.Item>Option Two</PopUpMenu.Item>
-          <PopUpMenu.Item>Option Three</PopUpMenu.Item>
+          <PopUpMenu.Item onClick={() => {}}>Option One</PopUpMenu.Item>


I don't see these as required in props, why do we need to add these here?

They aren't marked as required, but the item will through an exception if an href/onclick is not provided.

caleybrock · 2017-11-16T19:48:35Z

apps/src/templates/teacherDashboard/SectionActionDropdown.jsx

+  };
+
+  // Menu open
+  open = () => {


We chatted offline about these functions being able to be pulled apart into a separate component to be used for pop up actions in tables.

I'll probably own in a separate PR.

islemaster

Great progress on this! It's a big change. Very good reuse of color constants and other style constants throughout. I like that we are updating and replacing some older components instead of just producing redundant ones.

My comments break down into style nits, some React gotchas, and I think there's some work we should do now to make testing more thorough. I see Caley has called out some places where we could break large components apart a bit more, I agree that's also a good thing to look at here.

islemaster · 2017-11-16T20:13:45Z

apps/src/lib/ui/PopUpMenu.jsx

+  }
+}
+
+PopUpMenu.MenuBreak = Radium(MenuBreak);


Does this component get any benefit from Radium? I don't see any style arrays, hover behaviors, or styles that need browser-prefixing.

If not, and especially because this has no props, this would make a great functional stateless component.

There is one ":hover" in here (line 177) so I would be hesitant to remove Radium.

But that's on the PopUpMenu.Item component, not on the PopUpMenu.MenuBreak component.

islemaster · 2017-11-16T20:15:29Z

apps/src/lib/ui/PopUpMenu.jsx

    first: PropTypes.bool,
    last: PropTypes.bool,
+    color: PropTypes.string,
+    heightDivisor: PropTypes.number,


Reading this props list without the component implementation, I don't really know what heightDivisor does. At least having a comment documenting this prop would be nice, so someone wanting to reuse Item wouldn't need to read its whole implementation.

islemaster · 2017-11-16T20:19:30Z

apps/src/templates/teacherDashboard/SectionActionDropdown.jsx

+        sectionName,
+        removeSection,
+        toggleSectionHidden,
+        importOrUpdateRoster} from './teacherSectionsRedux';


Nit: We usually avoid aligned indentation, would prefer

import { sectionCode, //... importOrUpdateRoster, } from './teacherSectionsRedux';

Bonus points if you can find the eslint rule that would enforce and/or autofix this 😁

islemaster · 2017-11-16T20:22:20Z

apps/src/templates/teacherDashboard/SectionActionDropdown.jsx

+  // Menu open
+  open = () => {
+    this.updateMenuLocation();
+    window.addEventListener("resize", this.updateMenuLocation);


Resize events can fire a ton during a window resize, so it can affect performance to do much work in response to them. It would be good to throttle the updateMenuLocation callback here.

islemaster · 2017-11-16T20:23:12Z

apps/src/templates/teacherDashboard/SectionActionDropdown.jsx

+  beforeClose = (_, resetPortalState) => {
+    resetPortalState();
+    this.setState({open: false});
+    window.setTimeout(() => this.setState({canOpen: true}), 0);


What's with the setTimeout(_, 0)? Would it work to make this the async callback from setState instead?

islemaster · 2017-11-16T20:51:41Z

dashboard/test/ui/features/SectionActionDropdown.feature

+    And I click selector "#ui-test-edit-section" once I see it
+    And I press the save button to create a new section
+
+  # This should adequately test section down


I don't understand this comment.

islemaster · 2017-11-16T20:52:16Z

dashboard/test/ui/features/SectionActionDropdown.feature

+    Given I am a teacher
+    And I am on "http://studio.code.org/"
+    And I create a new section
+    And I click selector ".fa-chevron-down" once I see it


I'd go ahead and make a "I open the section action dropdown" step that aliases this, all these tests would read better.

islemaster · 2017-11-16T20:52:34Z

dashboard/test/ui/features/SectionActionDropdown.feature

+    And I create a new section
+    And I click selector ".fa-chevron-down" once I see it
+    And I click selector "#ui-test-hide-section" once I see it
+    And I click selector ".ui-test-show-hide" once I see it


Could stop and check between these steps that we can't see the section after it's hidden.

islemaster · 2017-11-16T20:53:08Z

dashboard/test/ui/features/SectionActionDropdown.feature

+    And I click selector "#ui-test-hide-section" once I see it
+    And I click selector ".ui-test-show-hide" once I see it
+    And I click selector ".fa-chevron-down" once I see it
+    And I click selector "a:contains('Show Section')" once I see it


Again, let's assert a final state of the site - check that the section moved back up into the regular sections area.

islemaster · 2017-11-16T20:57:58Z

apps/test/unit/lib/ui/SettingsCogTest.js

        );
        expect(wrapper.text()).to.include(msg.enableMaker());
      });

-      it('renders with enable maker option if maker is available and disabled', () => {
+      it('renders with disable maker option if maker is available and ensabled', () => {


Typo: ensabled

epeach · 2017-11-17T23:48:13Z

@islemaster @caleybrock Ready for another review when you get a chance.

islemaster

Awesome. Let's ship this and do incremental improvements afterward. Great work!

caleybrock · 2017-11-18T00:10:53Z

👏 👏 so excited for this - it looks good!

islemaster · 2017-11-20T18:00:48Z

🎉

Fixed small issues

3e43a04

audreyclark assigned epeach and audreyclark Sep 29, 2017

Erin added 7 commits October 5, 2017 14:35

WIP - Adding functionality to action box

abae99d

Merge branch 'staging' into SectionActionBoxDropdown

c9631cf

Merge branch 'staging' into SectionActionBoxDropdown

0bdad05

Add pop-up modal feature

b6adad7

Code cleanliness and visible clickability

06a2939

Add ability to delete appropriate section. Clean up dead code.

4b54042

Merge branch 'staging' into SectionActionBoxDropdown

9ff790a

epeach changed the title ~~Fixed small issues~~ Section Action Dropdown Oct 13, 2017

Erin added 10 commits October 13, 2017 13:19

Add on-click ui change to section table action cell.

57b3037

Center overlay trigger in the section table

ded7429

Correct capitalization for testing

b832a2d

Add GC and Clever syncing

c6e3259

Update Print Certificates test after refactor

a24e3c2

Added confirm delete pop-up

eba3b24

Add redux store to section action dropdown story

02dc9ee

Section info update

81314a9

Refactor to separate the dropdown action from the list of actions. Un…

593789b

…it tests.

UI Tests and refactoring for reusability

fdb7814

epeach requested a review from islemaster October 18, 2017 18:56

Merge branch 'staging' into SectionActionBoxDropdown

bd343cf

islemaster reviewed Oct 18, 2017

View reviewed changes

Erin added 9 commits November 14, 2017 16:15

Remove SectionActionBox and add SectionActionDropdown story

c6bdf3e

Renaming clean-up

aa68067

Fix popupmenu test bug to account for additional tag layer

f519781

Move menu slightly to the left and update hover colors on the chevron…

d5b59bf

… trigger

Pop-up dialog spacing fix

75b282e

Update SettingsCogTest

b89d976

Merge branch 'staging' into SectionActionBoxDropdown

72ff751

Fix Print Certificates test

6304e56

Update section action dropdown test

cea3dc6

caleybrock reviewed Nov 16, 2017

View reviewed changes

Clean-up styling

f37384b

islemaster reviewed Nov 16, 2017

View reviewed changes

Erin added 5 commits November 16, 2017 14:33

Update SectionActionDropdown feature

0da095d

Remove Radium from Menu Break

d49cb78

Code cleanliness

25599a0

Throttle and Template String fixes

e6f35d2

Update tests for cleanliness and structure

a7cf7c5

Improve step name

ac972ab

islemaster approved these changes Nov 18, 2017

View reviewed changes

epeach merged commit 2496471 into staging Nov 20, 2017

epeach mentioned this pull request Nov 20, 2017

Revert "Section Action Dropdown" #19317

Merged

epeach deleted the SectionActionBoxDropdown branch November 21, 2017 22:45

Section Action Dropdown #18058

Section Action Dropdown #18058

Conversation

audreyclark commented Sep 29, 2017 • edited by epeach

islemaster commented Oct 18, 2017

islemaster commented Oct 18, 2017

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster commented Oct 18, 2017

islemaster commented Oct 18, 2017

islemaster commented Oct 18, 2017 • edited

Choose a reason for hiding this comment

epeach commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islemaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epeach commented Nov 17, 2017

islemaster left a comment

Choose a reason for hiding this comment

caleybrock commented Nov 18, 2017

islemaster commented Nov 20, 2017

audreyclark commented Sep 29, 2017 •

edited by epeach

islemaster commented Oct 18, 2017 •

edited