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

Fix: Background Description was required but not always saved #55

Merged

Conversation

bradenmacdonald
Copy link
Contributor

This PR fixes the following bugs with the new drag and drop component:

  1. "Background description" was marked as required, but if you filled it out and pressed "Continue", "Save", it would accept the data but not actually save. Then next time you went to edit, the description would be blank and you'd get an error saying the field is required. (Fix was to always save the description field - not to only save it when users click on the "Change Background" button)
  2. When configuring the draggable items, the "Image Description" was always required, even if the "Image URL" was blank. With this fix, the image description is only required if the Image URL is not blank.
    screen shot 2016-02-09 at 3 50 09 pm
  3. When clicking certain action links in the dndv2 editor (e.g. "Add a Zone"), the browser would scroll to the top of the page (since the href="#" event was not prevented).
  4. When changing tabs in the dndv2 editor, the next tab would often be scrolled down halfway. Fixed so that when you go to the next tab, the editor scrolls to the top of the new tab.
  5. In Studio, Newly added drag and drop components do not load properly, due to a Studio bug. I've added a workaround since the fix (https://github.com/edx/edx-platform/pull/11433) is not yet merged and won't be on Cypress or Dogwood.

JIRA: YONK-264 / SOL-1642

Sandbox: http://studio.master.dndv2.sandbox.opencraft.com/ and http://master.dndv2.sandbox.opencraft.com/ (shared by a few different open PRs, but it does currently include the changes from this PR)

@bradenmacdonald
Copy link
Contributor Author

@dianakhuang Can you please provide a second review for this PR too? (next week is OK if you're not available this week). This (and the first PR I also just pinged you on) are some follow-up fixes to bugs found after #34 merged.

FWIW, the first two bugs in this PR were introduced during the a11y review on #34, after you'd already given thumbs up.

@mtyaka
Copy link
Contributor

mtyaka commented Feb 11, 2016

Looks good 👍

@bradenmacdonald
Copy link
Contributor Author

@mtyaka I just pulled in one more one-line workaround, FYI - it was already on the sandbox. Without that fix, newly-added drag and drop components do not load in Studio until you refresh the page. We need this workaround until https://github.com/edx/edx-platform/pull/11433 is reviewed and merged.

@@ -283,6 +283,7 @@ function DragAndDropBlock(runtime, element, configuration) {
if (!window.gettext) { window.gettext = function gettext_stub(string) { return string; }; }

var $element = $(element);
element = $element[0]; // <- workaround Studio bug until https://github.com/edx/edx-platform/pull/11433 is merged

Choose a reason for hiding this comment

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

It looks like the edx-platform PR merged, so I don't think you need this workaround anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dianakhuang Do you think it's ok if we leave it in for a while though, in order to preserve Dogwood compatibility? We have clients who are using this block on Dogwood.

Choose a reason for hiding this comment

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

I think it's fine to leave it in for compatibility reasons, but in that case, it'd be nice to have the comment updated to reflect exactly what needs to happen before it can be removed. I might be helpful to write the new comment as a // TODO: as well to make it easier to find later and let people know that it's an action that should be taken later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, makes sense. I will update the comment accordingly before merging this.

@dianakhuang
Copy link

Other than that, 👍

@bradenmacdonald
Copy link
Contributor Author

Thanks for the review, @dianakhuang ! I've updated that comment and rebased. Will merge once the build is green.

bradenmacdonald added a commit that referenced this pull request Feb 18, 2016
Fix: Background Description was required but not always saved
@bradenmacdonald bradenmacdonald merged commit 7b38b2e into openedx:master Feb 18, 2016
@bradenmacdonald bradenmacdonald deleted the braden/required-field-fixes branch February 18, 2016 19:25
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