Skip to content

Add a view for the field type Dict in Studio. - #2478

Merged
polesye merged 1 commit into
masterfrom
anton/add-view-for-dict-in-studio
Feb 11, 2014
Merged

Add a view for the field type Dict in Studio.#2478
polesye merged 1 commit into
masterfrom
anton/add-view-for-dict-in-studio

Conversation

@polesye

@polesye polesye commented Feb 5, 2014

Copy link
Copy Markdown
Contributor

In this PR is added a view for the field type Dict in Studio.
Ticket: BLD-658.
@cahrens , @frrrances please review.


screen shot 2014-02-05 at 14 08 50

@cahrens

cahrens commented Feb 5, 2014

Copy link
Copy Markdown

Thanks for adding this, Anton. We definitely need a Dict view.

Here are some thoughts from having added a dummy Dict setting to video_module so I could try out the UI:

  1. The entries appear in seemingly random order. That is to say, if I have a dict with 3 different key/value pairs, they do not appear in the same order each time I open the Settings dialog. I know that order does not matter, but this could be confusing/disconcerting to users. Would it be possible to display them in alphabetical order?

  2. The position of the revert button above the list seems odd.

  3. I agree that more space would be helpful. @frrrances what would you think of a change to put help strings beneath the elements instead of to the right (in general)?

  4. What do you want the "empty" behavior to be? It's possible currently to save entries with empty keys and values. Is that desirable? In contrast, the List view doesn't allow you to save an empty entry.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Forgot to change description (copied from List?).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Forgot to change description (copied from List?).

Yes, because it has pretty similar behavior. Fixed. Thanks.

@polesye

polesye commented Feb 6, 2014

Copy link
Copy Markdown
Contributor Author
  1. The entries appear in seemingly random order. That is to say, if I have a dict with 3 different key/value pairs, they do not appear in the same order each time I open the Settings dialog. I know that order does not matter, but this could be confusing/disconcerting to users. Would it be possible to display them in alphabetical order?

I believe the user will be more confused when he changes the key or adds a new value, fields will reorder.

  1. The position of the revert button above the list seems odd.

Agree.

  1. I agree that more space would be helpful. @frrrances what would you think of a change to put help strings beneath the elements instead of to the right (in general)?

Or beneath the labels.

  1. What do you want the "empty" behavior to be? It's possible currently to save entries with empty keys and values. Is that desirable? In contrast, the List view doesn't allow you to save an empty entry.

Fixed. Thanks.

@cahrens

cahrens commented Feb 6, 2014

Copy link
Copy Markdown

FYI, @frrrances is on vacation. I think she returns tomorrow.

Comment thread cms/static/sass/views/_unit.scss Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would just use the basic width here since height isn't needed:

width: 45%;

@frrrances

Copy link
Copy Markdown
Contributor

Nice work, @polesye. I put a few line comments in, but once you address those I think this looks good. 👍

As a side note, I agree with @cahrens that we should probably revisit the placement of the help text and the space given to the inputs sometime soon, though I don't think it belongs in this PR.

@cahrens

cahrens commented Feb 7, 2014

Copy link
Copy Markdown

There is a bug with the Add button getting disabled if you press Revert with an empty entry present (similar to BLD-821).

@frrrances

Copy link
Copy Markdown
Contributor

@polesye The placement of the revert/clear button is kind of troublesome, and the tight spacing of the two input fields isn't great either... Ideally, I would like to clean up the editor as we mentioned which would solve for both of those problems, but will take a little bit of time. Can you let me know whether the timeline on this would allow for another week for cleanup?

@polesye

polesye commented Feb 8, 2014

Copy link
Copy Markdown
Contributor Author

There is a bug with the Add button getting disabled if you press Revert with an empty entry present (similar to BLD-821).

@cahrens I added a fix for both editors (List and Dict).

@polesye

polesye commented Feb 8, 2014

Copy link
Copy Markdown
Contributor Author

@frrrances your comments are addressed.

Can you let me know whether the timeline on this would allow for another week for cleanup?

It'll block us. Probably, it will be better to clean up this editor in separate PR.

@cahrens

cahrens commented Feb 10, 2014

Copy link
Copy Markdown

@polesye It would be nice to have a unit test covering the bug fix (a test that fails without your change).

I'm seeing some strange shifting of text in the settings "modal" when I move the mouse over elements. I'll show it to @frrrances to see if she has an idea what is causing it. This is on Chrome/Ubuntu. An example is if I put my mouse over the "Add" button, the + in front of Add shifts to the left and then back to the right. Also, the help text shifts.

@polesye

polesye commented Feb 10, 2014

Copy link
Copy Markdown
Contributor Author

It would be nice to have a unit test covering the bug fix (a test that fails without your change).

@cahrens done.

@cahrens

cahrens commented Feb 10, 2014

Copy link
Copy Markdown

Thanks. I showed the shifting problem to @frrrances and she is going to look. Hopefully it is reproducible for other people!

@cahrens

cahrens commented Feb 10, 2014

Copy link
Copy Markdown

Turns out the "shifting" bug is existing on master.
https://edx-wiki.atlassian.net/browse/STUD-1296

👍 After rebasing to squash commits. Would love to see in the future the revert button move locations and a rework of the dialog for more room.

@frrrances

Copy link
Copy Markdown
Contributor

@polesye I moved the revert button down and made a few css adjustments to give the dict more room and handle the awkward layout. (if you see any issues with those changes, feel free to revert them or chat with me tomorrow.) either way, 👍

polesye added a commit that referenced this pull request Feb 11, 2014
Add a view for the field type Dict in Studio.
@polesye
polesye merged commit 0b3604f into master Feb 11, 2014
@polesye
polesye deleted the anton/add-view-for-dict-in-studio branch February 11, 2014 13:45
@cahrens

cahrens commented Feb 11, 2014

Copy link
Copy Markdown

@mhoeber This PR changed formatting of settings editor for components (video, problem, html, etc.). You may need to update screen shots in the documentation.

It will be in next week's release.

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.

3 participants