Skip to content

Badge listing UI - #10861

Merged
Kelketek merged 1 commit into
feature/badges-v2from
kelketek/badges-v2-ui
Dec 21, 2015
Merged

Badge listing UI#10861
Kelketek merged 1 commit into
feature/badges-v2from
kelketek/badges-v2-ui

Conversation

@Kelketek

@Kelketek Kelketek commented Dec 7, 2015

Copy link
Copy Markdown
Contributor

Description: Provides a UI for viewing badges in a user's profile.
Jira: https://openedx.atlassian.net/browse/SOL-1365
Dependencies: https://github.com/edx/edx-platform/pull/10732
Sandbox http://pr10556.sandbox.opencraft.com/

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Kelketek! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?number=10861&repo=edx%2Fedx-platform

@Kelketek Kelketek mentioned this pull request Dec 8, 2015

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.

FYI, per Django docs, another option "If you want to be able to modify this field, set the following instead of auto_now_add=True: default=timezone.now - from django.utils.timezone.now()"

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.

@bradenmacdonald Yes, but aside from this initial use, it shouldn't need manual setting again.

@Kelketek
Kelketek force-pushed the kelketek/badges-v2-ui branch from 8502e94 to 33defc8 Compare December 9, 2015 05:30
@Kelketek

Kelketek commented Dec 9, 2015

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald I have this to the point where it displays the badges in the user's profile. To try it out, go log in as the honor user and view your profile page.

@talbs I'm reserving the share button and modal for a follow-up PR since I didn't get a chance to hit it today with all the fixes needed for this and the API. So I'd like to get the basic view layout looked over at your first opportunity while I work on the rest.

@nasthagiri This PR modifies the User account API slightly to allow the badges to show up in the profile. Please let me know if these modifications are acceptable. They're in edx@44f3dcf

@talbs

talbs commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

@Kelketek is there a sandbox view that I can take a look at?

@Kelketek

Kelketek commented Dec 9, 2015

Copy link
Copy Markdown
Contributor Author

@talbs

talbs commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

@talbs http://pr10556.sandbox.opencraft.com/

Thanks much, @Kelketek. Is there a particular user on that sandbox that has earned badges or a way to get to the view you've built out with this work?

@talbs

talbs commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

Nevermind, just found it under honor@example.com. Sorry!

@talbs

talbs commented Dec 9, 2015

Copy link
Copy Markdown
Contributor

@Kelketek, thanks for the support in reviewing.

profile badges-compare

^ Comparing the static comp/visual design with your rendering, there are a few things that need some adjustment:

  • The spacing between the new tabs you added and the profile image/details seems small.
  • Font-size of the tab text seems small
  • Font-sizes of the date/copy text of badges should be different
  • Line-height of the details text seems less than what's expected
  • The "Earn your next accomplishment" call to action area is missing the background/border/box-shadow treatment in the comp
  • The empty badge "Earn your next accomplishment" dotted pattern is darker than expected and has a space at the top of it.
  • Smaller things include: spacing between tab text and borders; vertical spacing between the two columns seems tight.

Hope that visual design feedback helps.

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.

Is this a requirement? I don't see mention of it in UX-2980. IMHO it's confusing to hide this sometimes, and it means users with no badges won't see the "Earn Your Next Accomplishment" call to action.

@Kelketek Kelketek mentioned this pull request Dec 9, 2015
6 tasks
@Kelketek
Kelketek force-pushed the kelketek/badges-v2-ui branch from 33defc8 to a4e5d61 Compare December 9, 2015 22:21
@bradenmacdonald

Copy link
Copy Markdown
Contributor

@Kelketek Not sure if this will ever happen, and you might have already fixed it, but: if someone uploads a rectangular badge image, the layout runs together and doesn't have enough padding:
screen shot 2015-12-09 at 4 22 12 pm

Should be fixable by adding padding and/or margin to the .badge-display

@Kelketek

Copy link
Copy Markdown
Contributor Author

@bradenmacdonald That should not be possible as a rectangular image would be rejected normally.

@talbs This is ready for a second look from you.

@catong I don't think there's likely to be any significant changes to strings/error messages in this PR from this point forward, so you should be able to take a look at this now.

@Kelketek
Kelketek force-pushed the kelketek/badges-v2-ui branch from 02c82c5 to c56290d Compare December 10, 2015 04:21
Comment thread lms/djangoapps/badges/models.py 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.

Consider using TimeStampedModel instead from model_utils.models. You can see other uses of it in edx-platform.

@talbs

talbs commented Dec 10, 2015

Copy link
Copy Markdown
Contributor

@Kelketek, things look generally good. One small thing, .badge-date-stamp should have a font-size of 14px, not 12px.

@talbs

talbs commented Dec 10, 2015

Copy link
Copy Markdown
Contributor

@Kelketek, once you address that small visual change, 👍

@Kelketek

Copy link
Copy Markdown
Contributor Author

@nasthagiri Ready for another look here.

@Kelketek

Copy link
Copy Markdown
Contributor Author

@catong Strings updated.

@bradenmacdonald

Copy link
Copy Markdown
Contributor

👍 LGTM

@Kelketek Kelketek changed the title (WIP) Badge listing UI Badge listing UI Dec 11, 2015

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.

Nit: this should use title case, i.e. "Accomplishments Pagination"

@andy-armstrong

Copy link
Copy Markdown
Contributor

@Kelketek This is an excellent feature, and I love how it is all built using reusable components. I have a few nits and some suggestions for the unit tests, but otherwise this looks great. Let me know when it is ready for my second review.

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.

@Kelketek I recommend using something that global audiences will understand in the example: "hat_trick" -> "completed_3_courses".

@Kelketek

Copy link
Copy Markdown
Contributor Author

@andy-armstrong I'm working on your notes. I wasn't aware of the Teams project and the components it added until after most of the reviews were completed-- when I was asked to paginate the badging results, when I ran across it and had to patch up the pagination component to make it more generally usable.

The tabbed view component I've just now discovered as of a few moments ago. Looks like my ModeToggle view is redundant. It's going to take time to refactor everything to use the existing view (which includes history and, thus, the ability to link directly to badges). I'll be pushing fixes for the other issues first, then I suppose I'll get to redoing a bunch of stuff.

@talbs

talbs commented Dec 17, 2015

Copy link
Copy Markdown
Contributor

@Kelketek, https://github.com/edx/edx-platform/pull/10997 has been merged. Can you refactor your styling/markup to leverage this new app-wide pagination element? Let me know the timing on that as well as when I can help confirm that you've removed any of the Sass you've had to write/port yourself.

Thanks much!

@andy-armstrong

Copy link
Copy Markdown
Contributor

@Kelketek One thing I forgot to mention yesterday: should the badges be ordered reverse chronologically? It seems odd to show my oldest badges first, especially if I've been using edX for a number of years. I think it would make sense to show the most recent ones first.

@Kelketek
Kelketek force-pushed the kelketek/badges-v2-ui branch from d4d0da7 to 0996f25 Compare December 17, 2015 23:28
@Kelketek
Kelketek force-pushed the feature/badges-v2 branch 3 times, most recently from c63830c to fecaad5 Compare December 18, 2015 04:24
@Kelketek
Kelketek force-pushed the kelketek/badges-v2-ui branch from c7267b8 to 0c249aa Compare December 18, 2015 04:25
@Kelketek

Copy link
Copy Markdown
Contributor Author

I've had to rebase this to get @talbs 's request of incorperating the recently merged pagination SASS chances done.

@catong You will find the updates you requested in edx@bfc1956

120f1cb Is the start of the most recent wave of changes. It took all day to refactor the views to use the Team's tabbed view (I rather wish I had known about that beforehand instead of just before we were done). I will not be able to make the badges reflow quite as nicely as we'd like-- we don't have a proper grid system in the LMS that can be used consistently and we do not have time to implement one now-- in fact we really don't have time to do much else with this PR.

@andy-armstrong Please give this another look.

@Kelketek Kelketek mentioned this pull request Dec 18, 2015
@Kelketek

Copy link
Copy Markdown
Contributor Author

In the interest of expediency, the sandbox has been updated with the share modal for https://github.com/edx/edx-platform/pull/11028 . It includes all of the current work here, though.

@andy-armstrong

Copy link
Copy Markdown
Contributor

@Kelketek Thanks so much for the extensive reworking, and sorry you weren't aware of some of these features until late in the day. 👍 from me once the hard-coded "edX" string has been removed.

@talbs

talbs commented Dec 21, 2015

Copy link
Copy Markdown
Contributor

@Kelketek, you still have my 👍 for this. Thanks for the extra syncing up with our recent changes.

@Kelketek
Kelketek force-pushed the kelketek/badges-v2-ui branch from 0dc9e97 to cf5ab04 Compare December 21, 2015 19:40
@Kelketek
Kelketek merged commit cf5ab04 into feature/badges-v2 Dec 21, 2015
@Kelketek
Kelketek deleted the kelketek/badges-v2-ui branch December 21, 2015 19:41
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Kelketek! I've created OSPR-1567 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

If you like, you can add yourself to the AUTHORS file for this repo, though that isn't required. Please see the CONTRIBUTING file for more information.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs triage open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants