Skip to content

Conversation

@symbolist
Copy link
Contributor

https://openedx.atlassian.net/browse/TNL-1499
https://openedx.atlassian.net/browse/TNL-1534

The "Connected Accounts" functionality will be implemented in a separate PR.

@symbolist symbolist force-pushed the usman/tnl1499-account-settings branch from dcc122c to 6d7372e Compare March 13, 2015 17:47
@symbolist
Copy link
Contributor Author

@explorerleslie @catong @marcotuts The sandbox should meet all the requirements now. Please review: http://learnerprofile.m.sandbox.edx.org/

@catong
Copy link
Contributor

catong commented Mar 16, 2015

@symbolist, please tag @srpearce. She is the writer for this feature and should also be tagged for any doc strings/UI review. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

This should have its own section outside of the //shared - course area I think. I would make a "// views - platform" section underneath shared - course

Additionally, this import might make sense to add to application-extend2.scss.mako since that's where the other platform level views are imported.

@symbolist symbolist force-pushed the usman/tnl1499-account-settings branch from 36448eb to ec538e7 Compare March 17, 2015 12:28
Copy link
Contributor

Choose a reason for hiding this comment

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

anytime there's a math operation on a css variable, our convention is to wrap these in parentheses. ($baseline*2) I'll note this when I see it on this PR.

@symbolist symbolist force-pushed the usman/tnl1499-account-settings branch from ec538e7 to acb59d2 Compare March 17, 2015 15:09
@symbolist
Copy link
Contributor Author

@explorerleslie
Copy link

@symbolist where is the list of languages that you're using come from? is it a default python library? I'd prefer a list the same as the one we use in video transcript language selection:

image

@symbolist symbolist force-pushed the usman/tnl1499-account-settings branch from acb59d2 to 6f7c590 Compare March 17, 2015 18:11
@symbolist
Copy link
Contributor Author

@explorerleslie
Copy link

@symbolist I see languages like LOLCATS, Pirate, and Dummy Language on the account settings page that aren't in the list that you sent me. Why is that? Can we use the same list as in Studio?

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: remove newline

@marcotuts
Copy link
Contributor

@symbolist I continued my styling / FED feedback through this PR / branch - https://github.com/edx/edx-platform/pull/7400

In summary, the suggested changes including some naming conventions for class names, table of contents comments for the scss file, text alignment for the fields even in case of longer support strings, and the use of borders as visual separators instead of


tags

@explorerleslie
Copy link

@cptvitamin could you please do an accessibility review? Sandbox is http://userprofile.m.sandbox.edx.org/account/settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

this <label> element is not programmatically associated with the form field it relates to. Either the form field needs to be a child of the <label> or the <label> element needs a for= attribute that references the ID of the associated form field. Also, the <label> needs to be associated with the *field-message span that follows with an aria-describedby= attribute that references an ID associated with that <span>

@cptvitamin
Copy link
Contributor

@symbolist @explorerleslie, I did a first pass at an accessibility review. Would love another pass at it once these changes are implemented.

@andy-armstrong andy-armstrong force-pushed the usman/tnl1499-account-settings branch from 3acba0d to 3b37766 Compare April 16, 2015 21:54
@cahrens
Copy link

cahrens commented Apr 17, 2015

@andy-armstrong andy-armstrong force-pushed the usman/tnl1499-account-settings branch from db9f4ba to bb083b5 Compare April 17, 2015 15:22
@andy-armstrong andy-armstrong force-pushed the usman/tnl1499-account-settings branch from bb083b5 to 5085f94 Compare April 17, 2015 18:07
@andy-armstrong
Copy link
Contributor

There was a strange acceptance failure here so I'm going to kick off another build:

19:12:11 ERROR:util.models:String decompression failed. There may be corrupted data in the database: Incorrect padding
19:12:37 Build timed out (after 60 minutes). Marking the build as aborted.

https://build.testeng.edx.org/job/edx-platform-test-subset/40023/console

@andy-armstrong
Copy link
Contributor

Jenkins, please test this

@andy-armstrong
Copy link
Contributor

Jenkins, test this please

@cahrens
Copy link

cahrens commented Apr 17, 2015

All tests have passed, merging!

cahrens pushed a commit that referenced this pull request Apr 17, 2015
@cahrens cahrens merged commit 958fb00 into learner-profiles Apr 17, 2015
@andy-armstrong
Copy link
Contributor

Yay!

@cahrens
Copy link

cahrens commented Apr 17, 2015

Look, it's a green checkmark!!

@andy-armstrong andy-armstrong deleted the usman/tnl1499-account-settings branch June 4, 2015 17:11
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.