Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Studio: Authorship Rights Request UI + Dashboard Clean-up #341

Merged
merged 40 commits into from
Jul 26, 2013

Conversation

talbs
Copy link
Contributor

@talbs talbs commented Jul 7, 2013

This working branch adds in some additional UI to a Studio User's Dashboard and include the following changes:

  • static representations of all possible dashboard states (including with new authorship rights request UI)
  • general UI clean-up for all dashboard states (right hand sidebar, help text, logical welcome greetings depending on state)
  • re-architected system-help Sass to handle styling of some of the cleanup UI (introductions, status, confirmation notices)

@cahrens, as I mentioned in person, I could use your help to put the proper logic in place to render the proper UI for each use case/scenario. Would you mind as part of your wiring up, trying to abstract this very static mako template? I'm happy to help, but need to know some of the parameters we can use for if statements within the template logic. Feel free to use this branch as your working one as well - I'll be hands-off until any revisions from the group are needed.

@markchang, please take a look through these states and make sure they cover the scenarios we're expecting. Also, I could use your help in producing content for the areas where there is now guided placeholder copy. Mind adding that in via comments here or directly into the branch?

@frrrances, mind taking a spin through my new semantics, markup, and Sass additions to make sure they make sense?

Thanks for your help.

@markchang
Copy link
Contributor

@cahrens can we actually re-send the email verification?

@markchang
Copy link
Contributor

@talbs can we move the "request authorship" block to above the list of courses? What do you think?

@talbs
Copy link
Contributor Author

talbs commented Jul 8, 2013

@markchang, sure - I'm happy to move that UI element above course listings.

You previously mentioned that this feature/request mechanism should be downplayed and not prominently presented to users (which is why I added it below courses). With that in mind, you still cool with moving it?

@markchang
Copy link
Contributor

This is true. I will adjust the copy sometime before we launch.

@talbs
Copy link
Contributor Author

talbs commented Jul 8, 2013

Thx, @markchang. So, should I go ahead and move those UI elements above courses or are we downplaying them by keeping them where they are?

@talbs
Copy link
Contributor Author

talbs commented Jul 8, 2013

@cahrens, also, I think I may have broke the "Create/Add a Course" form showing up within the DOM with this static representation. If its something easy to fix in the JS go for it, otherwise I can try to take a look once we have the page's logic in place.

@markchang
Copy link
Contributor

@talbs leave it in place and one of us should update the copy to point in the right direction (above vs. below)

@talbs
Copy link
Contributor Author

talbs commented Jul 8, 2013

Thanks for the info - I made that quick copy change in all of the states. Hope that helps.

<h2 class="title">${_("Welcome, %(name)s!") % dict(name= user.username)}</h2>

<div class="copy">
<p>${_("You don't have any courses you're working on in Studio yet.")}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of this wording. Can we get away with something like "You haven't created any courses in Studio yet."?

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me. Someone change plz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presto! Change-O! Copy edited.

@talbs
Copy link
Contributor Author

talbs commented Jul 8, 2013

Okay, revised copy and overwrote some animations - going hands-off until post Hackathon.

@cahrens
Copy link

cahrens commented Jul 9, 2013

@markchang No, we cannot easily resend the e-mail verification. It should be another story.

@talbs
Copy link
Contributor Author

talbs commented Jul 10, 2013

After talking with @cahrens, I'm going to keep this branch for future efforts, but will be pairing down some of the Dashboard UI changes to be scoped only to authorship rights in a separate branch.

<h1>${_("Activation Invalid")}</h1>
<div class="wrapper-mast wrapper sr">
<header class="mast">
<h1 class="page-header">${_("Studio Account Activation")}</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Product name? Translatable or not?

@talbs
Copy link
Contributor Author

talbs commented Jul 25, 2013

@chrisndodge thanks for the comment about the # of commits. Are there particular standards we should adhere to with commits? I'm guessing a lot of my commits are thickening the history here and am happy to condense. I'd just love: 1) to understand what the standards/guidelines are for commit history now and 2) how to trim down the commits (I think the whole design team could use a walkthrough).

Any chance you have answers on those two items?

@chrisndodge
Copy link
Contributor

@cahrens excellent coding job (as usual)!

Two things:

  • I believe we're supposed to use .format() for string formatting now and not '%'. Dave O has flagged this for me in the past, so maybe we should all agree on such a standard, if that is what is expected
  • I18N: Can we get consensus on whether trademarks and product names should be outside of translatable strings? There are a lot of references to 'edX' and 'Studio' in this PR. I stopped flagging them after awhile....

Thanks

@chrisndodge
Copy link
Contributor

@talbs I don't know of any guidelines, but I've been asked in the past to use 'rebase -i' to shorten up the commit history. If I'm reading this history correctly, where there a few merges from master as well?

Maybe some of the git-heads (@cpennington @gwprice) might be able to better articulate what they believe to be good form. Maybe this just gets let through this time, but they can set some wider Dev standards, if they feel appropriate?

@cpennington
Copy link
Contributor

The big red flags to me the several duplicate copies of the same commits, and the merge in from master (I'd rather see it as a rebase on top of master).

If there are multiple commits that are really just revisions of the same logical change, then those would ideally be merged together, but I'm less qualified to judge whether the commits are actually in a good state in that regard in their current form.

@cahrens
Copy link

cahrens commented Jul 25, 2013

@chrisndodge No, I don't want to rebase -i. I do not think it is worth the effort, and it is not a required part of our PR process. I think it is more important that the commit messages be helpful/correct (ie, no "checking in work" or "fixing some stuff") than that there be a small number of commits. I've been much more careful about my commit messages. The reason this branch is doing merges instead of rebases is because Brian and I are working on it together.

@cahrens
Copy link

cahrens commented Jul 25, 2013

@chrisndodge In the "big" i18n request that @singingwolfboy shepherded through, we decided to not try to pull edX and Studio out of translatable strings. It seemed too difficult, and instead we were hoping on guidelines for the translators about what is and is not translatable. It's great this discussion is continuing, as I am also concerned about what will happen in translation. However, for now I will stick with the decision we made in that pull request.

@cahrens
Copy link

cahrens commented Jul 25, 2013

@chrisndodge @talbs has addressed Frances' comments, and I addressed those of yours that I plan to do. :) I am seeking a thumbs-up.

@chrisndodge
Copy link
Contributor

+1 LGTM

cahrens pushed a commit that referenced this pull request Jul 26, 2013
Studio: Authorship Rights Request UI + Dashboard Clean-up
@cahrens cahrens merged commit 166aea6 into master Jul 26, 2013
@jzoldak jzoldak deleted the talbs/studio-authorship branch May 5, 2014 15:00
martynjames pushed a commit that referenced this pull request Dec 5, 2014
SOL-129 Remove Text category filter and add MS contentTypes
hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Dec 14, 2015
hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Dec 14, 2015
diegomillan pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2016
* xblock/image-coding:
  Install OLI Image Coding XBlock
xavierchan pushed a commit to xavierchan/edx-platform-1 that referenced this pull request Apr 3, 2019
Agrendalath pushed a commit that referenced this pull request Aug 18, 2021
[SE-3248] Revert `progress_video` event changes
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
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.

6 participants