Skip to content

Conversation

@frrrances
Copy link
Contributor

This is a FED cleanup of the Studio Outline page. @talbs and @cahrens can you review?

@talbs
Copy link
Contributor

talbs commented Dec 9, 2013

@frrrances, I'm still going through - but I noticed I can't cancel out of creating a new subsection (cancel button doesn't fire any events).

@talbs
Copy link
Contributor

talbs commented Dec 9, 2013

@frrrances, also, it looks like the modal depths visuals are applied backwards (the modal window has the blur filter applied and the background is clear). See this screengrab:

screen shot 2013-12-09 at 3 37 14 pm

@talbs
Copy link
Contributor

talbs commented Dec 9, 2013

@frrrances, the padding of the "New Section" UI looks like it was increased with this work (see screengrab). Can you tighten this up?

screen shot 2013-12-09 at 3 42 39 pm

@talbs
Copy link
Contributor

talbs commented Dec 9, 2013

@frrrances, also, looks like the "Collapse/Expand All Sections" visually toggles something (height or padding) with the "New Section" UI - not sure if this was happening before or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have elements/_modal.scss. While the Sass in there is part of the older base we want to refactor, should the contents of this file be included there for future maintenance rather than another file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This is meant to be part of the subset of modals for form actions, so I wanted to use the _uploads.scss file but it isn't named well, so I made a new one and we should merge them into a single _modal-forms.scss file. For now is it okay to leave this as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

any way to clean up this value so we don't have to use a hard-coded hex value and an !important rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. updated.

@talbs
Copy link
Contributor

talbs commented Dec 9, 2013

@frrrances, very nice work cleaning up this view visually and technically. This makes a huge difference and sets us up really well to continue to make this part of Studio even better.

I've looked through things and all of my comments, suggestions, and questions are inline. Let me know if you want to talk anything over or have any concerns. I'll take a spin through once you're done making any revisions for a final review.

@frrrances
Copy link
Contributor Author

@talbs, i think I have addressed all of your comments aside from a few css rule orders that seemed okay. Feel free to take another spin through to make sure I've handled all your comments. Thanks for the thorough review!

@cahrens I think this is good for you to review now.

@talbs
Copy link
Contributor

talbs commented Dec 10, 2013

Thanks for all of the attention to detail, @frrrances. This looks good - please ship this enormous improvement ASAP.

👍

Copy link

Choose a reason for hiding this comment

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

Delete commented out code and file dependency on ModalUtils.

@cahrens
Copy link

cahrens commented Dec 12, 2013

Three things:

  1. Drop indicator when moving sections is off (too far to the right, extends past sections).
  2. No spacing between units on the RHS navigator on Unit page (note that I'm on Chrome, Ubuntu).
  3. Drop indicators between units and subsections are duplicated. This is noticeable when you drag slowly.

@cahrens
Copy link

cahrens commented Dec 12, 2013

Another thing-- you have lost the indications of private and public on the subsection page. Every unit shows up as a blue hyperlink.

Since this PR is a pretty big change (and it's gone through several iterations), I would recommend that you have Raees do some manual testing on it. This just means creating a wiki testing page, talking to Jay, and then coordinating with Raees.

Copy link

Choose a reason for hiding this comment

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

Don't think you need data-parent and data-id here. Was there a reason you added these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was imitating the HTML... Jay said it probably didn't matter but it was nice to have it be as close as possible so i added it.

Copy link

Choose a reason for hiding this comment

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

data-id should no longer be in the HTML. data-parent may be. Can you check on both? And if you are keeping data-parent, fix the typo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. excellent. unfortunately my branch was before and after some of don's work so i tried to untangle those. i'll make sure data-id is gone and ask don about data-parent.

Copy link

Choose a reason for hiding this comment

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

I'm the one who did the work to remove data-id. I guess data-parent should stay here (though it really doesn't matter).

@ghost ghost assigned cahrens Dec 16, 2013
@jzoldak
Copy link
Contributor

jzoldak commented Dec 17, 2013

👍

frrrances pushed a commit that referenced this pull request Dec 17, 2013
@frrrances frrrances merged commit 8a23f43 into master Dec 17, 2013
@frrrances frrrances deleted the frances/fix/studio-outline-cleanup branch December 17, 2013 15:27
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request May 25, 2017
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.

5 participants