Skip to content

Conversation

@cahrens
Copy link

@cahrens cahrens commented Jan 16, 2014

@singingwolfboy and @andy-armstrong Please review.

@marcotuts Please check out my CSS changes.

STUD-1186

Copy link
Author

Choose a reason for hiding this comment

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

postJSON is defined in main.coffee. We always load that when running CMS, but we don't always load it when running unit tests (unless an earlier test loads it). I'm making the dependency more clearly stated.

@andy-armstrong
Copy link
Contributor

👍 Looks great, and those are awesome tests.

@marcotuts
Copy link
Contributor

I've branched off of this to add the visual styling changes for the component header, and should have something to review later today. I was planning on merging my branch into this one prior to merge of christina/duplicate into master, letting the work enter master as 1 PR.

If anybody feels I should instead treat the visual styling as a separate changelog item then I can open a pull request to master after this branch (as is) merges.

@cahrens
Copy link
Author

cahrens commented Jan 22, 2014

@marcotuts I would prefer to merge this PR first if it is ready to go. I'm still waiting on @singingwolfboy, as well as a thumbs-up from you. So I think it comes down to timing more than a philosophical stance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making the lettuce test use the zero-based index (awkward phrasing), can we use the terms "first", "second", "third", and so on? For example:

@step(u'I duplicate the (first|second|third) component$')
def duplicated_component(step, ordinal):
    ord_map = {
        "first": 0,
        "second": 1,
        "third": 2,
    }
    index = ord_map[ordinal]
    # and continue

Copy link
Author

Choose a reason for hiding this comment

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

I could, but I thought it was overkill (and not very extensible, as the mapping has to exist for each ordinal). Let me think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this call is unsuccessful?

Copy link
Author

Choose a reason for hiding this comment

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

The "Adding" or "Duplicating" notification is replaced by our standard "Studio is having trouble saving your work" message at the bottom. I don't add the component into the unit, under the assumption that the error meant the creation or duplication did not work.

In reality, it is possible that the creation or duplication did work, and the error occurred after that. In that case, refreshing the page (which we recommend in the error message) will show the component.

The code as previously written always assumed that the operation was successful-- the component was added to the page immediately, and the analytics tracking message was created, both before we even heard back from the server.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do:

gettext('Adding') + '…'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, nevermind. I don't know which is correct.

@singingwolfboy
Copy link
Contributor

👍

cahrens pushed a commit that referenced this pull request Jan 22, 2014
Front-end work for duplicating components on the unit page.
@cahrens cahrens merged commit 4544d27 into master Jan 22, 2014
@jzoldak jzoldak deleted the christina/duplicate branch May 5, 2014 14:54
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Oct 19, 2017
* fix add note to Zendesk help openedx#2194 (openedx#2202)

* fix add library option, and library links to the course. openedx#2181 (openedx#2205)

* second fix. add library option, and library links to the course. openedx#2210 (openedx#2211)

* third fix. add library option, and library links to the course. openedx#2213 (openedx#2214)

* Add the validation of the due date before 1900 openedx#2188 (openedx#2208)

* Fix bug for biz playback page title openedx#2195 (openedx#2203)

* fix test case. add library option, and library links to the course. openedx#2213 (openedx#2220)

* Add note to page of Password reset confirm. openedx#2216 (openedx#2217)

* Add login code to Playback Status page when the contract using login code openedx#2193 (openedx#2209)
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