separate design mode content out of divApplab by davidsbailey · Pull Request #4464 · code-dot-org/code-dot-org

davidsbailey · 2015-10-09T07:14:11Z

This PR creates a separate div to hold design mode contents. This protects us against a class of bugs where html generated in Code mode ends up in the levelHtml, and generally reduces the amount of logic that we need when switching between Design mode and Code mode.

designModeViz is the source of truth for the app's HTML. Applab.levelHtml is lazily updated from there (via serializeToLevelHtml). divApplab is then updated from Applab.levelHtml (via parseFromLevelHtml).

It might be possible to eliminate Applab.levelHtml entirely, but I didn't want to spend too much time fixing things that might not need to be fixed.

caveats:

element IDs are duplicated within divApplab and divDesignMode. This is "bad" but doesn't seem to cause any problems.

tested:

mochaTests (ec_design and ec_screens)
~/src/cdo/dashboard/test/ui$ ./runner.rb -d localhost.studio.code.org:3000 -f features/applab/scenarios.feature --html (skipped on IE and iProducts)
manually verified in Chrome/Mac that active screen has keyboard focus after Run button is pressed

init sequence cheat sheet:

Applab.init
  studioApp.init
    this.configureDom(config)
      container.innerHTML = config.html   // designModeViz created
    this.handleEditCode_
      config.afterInject
        drawDiv
        Applab.setLevelHtml
          designMode.parseFromLevelHtml(designModeViz);
          designMode.loadDefaultScreen();
          designMode.serializeFromLevelHtml();
    this.reset(true) // Applab.reset 
      designMode.parseFromLevelHtml(newDivApplab)

studioApp.runButtonClickWrapper
  Applab.serializeAndSave(callback);
    designMode.serializeToLevelHtml();
    callback → Applab.execute
      studioApp.reset(false); // Applab.reset
        designMode.parseFromLevelHtml(newDivApplab)
      Applab.loadDefaultScreen()

Bjvanminnen · 2015-10-09T16:04:01Z

Note: Looks like shared tests failed circleci.

Bjvanminnen · 2015-10-09T16:27:49Z

An argument in favor or using options hashes for params, is that this would be easier to read as

designMode.parseFromLevelHtml(designModeViz, { allowDragging: true });

If we were in ES6 land, we could then even declare our function as follows (specifying that we have a hash with a field of allowDragging)

designMode.parseFromLevelHtml = function (element, { allowDragging }) { // allowDragging is now a local variable }

In any case, no changes are necessary for this PR :)

Yeah, our code has lots of unidentifiable params being passed to functions. I've also seen these two conventions:

designMode.parseFromLevelHtml(designModeViz, true /* allowDragging */ );

or

var allowDragging = true; designMode.parseFromLevelHtml(designModeViz, allowDragging);

However (until we do ES6) I think I like the options params hash better since once you define the function that way, the caller is forced to make their call readable.

Bjvanminnen · 2015-10-09T16:56:53Z

The caveat scares me a little bit "element IDs are duplicated within divApplab and divDesignMode. This is "bad" but doesn't seem to cause any problems." I worry that not all browsers handle this the same. Probably worth some quick verification on different browsers.

I know you started looking into prepending something to all ids. Presumably that didn't end up being easy? It looks like we'll eventually do that work in https://www.pivotaltracker.com/story/show/103544734.

Bjvanminnen · 2015-10-09T17:02:02Z

Generally lgtm, tho I'd be sure to validate behavior on other browsers.

davidsbailey · 2015-10-09T17:58:29Z

I did not try prefixing. maybe worth doing now. how does this sound:

code_ prefix for elements in divApplab
design_ prefix for elements in designModeViz
no prefix for any HTML stored as strings (Applab.levelHtml, startHtml, level.levelHtml, etc.)

Bjvanminnen · 2015-10-09T18:02:24Z

Sounds good to me.

If you're reasonably confident that having duplicate ids doesn't break things on other browsers, I'm also okay with you getting what you have here committed, and then doing the prefixing at a future time in a separate item.

davidsbailey · 2015-10-11T00:48:16Z

@Bjvanminnen I realized I could cut the problem in half by only adding the prefix in design mode for now. It turned out to be a very good idea to perform this step, since I uncovered several places where we were referring to elements in divApplab via document.getElementById(), and several other places where I caught problems by asserting that an element is under designModeViz by calling checkId().

Please take another look. I may add some tests for the new elementUtils before merging.

davidsbailey · 2015-10-11T06:16:09Z

The NetSim failures in circle ci look legit.
On Oct 9, 2015 11:02 AM, "Bjvanminnen" notifications@github.com wrote:

Sounds good to me.

If you're reasonably confident that having duplicate ids doesn't break
things on other browsers, I'm also okay with you getting what you have here
committed, and then doing the prefixing at a future time in a separate item.

—
Reply to this email directly or view it on GitHub
#4464 (comment)
.

Bjvanminnen · 2015-10-12T16:37:38Z

Good stuff. Had a bunch of comments (most somewhat nitty) on the commits themselves.

davidsbailey · 2015-10-13T00:34:41Z

@Bjvanminnen please take another look. I've addressed all code review feedback and fixed a few regressions and test failures.

Bjvanminnen · 2015-10-13T01:32:45Z

changes lgtm. looks like circleci reports failure - not sure if these are legit

separate design mode content out of divApplab

commit 5cb0308 Merge: d1a3140 7ff9cb2 Author: davidsbailey <davidsbailey@users.noreply.github.com> Date: Tue Oct 13 07:21:54 2015 -0700 Merge pull request #4464 from code-dot-org/design-mode-viz separate design mode content out of divApplab commit 7ff9cb2 Author: David Bailey <dsb123@gmail.com> Date: Mon Oct 12 17:33:41 2015 -0700 fix dimming regression. add tests. commit d1a3140 Merge: 89530d2 bd42943 Author: Bjvanminnen <Bjvanminnen@gmail.com> Date: Mon Oct 12 17:30:49 2015 -0700 Merge pull request #4470 from code-dot-org/appsGitIgnore move apps .gitignore contents into root commit 89530d2 Merge: b9f6b17 74d18d1 Author: Bjvanminnen <Bjvanminnen@gmail.com> Date: Mon Oct 12 17:27:43 2015 -0700 Merge pull request #4468 from code-dot-org/under13Applab logged in under 13 cant acccess applab shares or /view commit b9f6b17 Author: Tanya Parker <tanyacparker@gmail.com> Date: Mon Oct 12 17:22:39 2015 -0700 update artist.feature to remove unnecessary space in error message commit 74d18d1 Author: Brent Van Minnen <bjvanminnen@gmail.com> Date: Fri Oct 9 12:32:30 2015 -0700 logged in under 13 cant acccess applab shares or /view commit f410bca Merge: 07ba0a0 abd05cf Author: Caley Brock <caleybrock7@gmail.com> Date: Mon Oct 12 17:07:33 2015 -0700 Merge pull request #4510 from code-dot-org/promote-pages Link state fact pages to code.org/promote page commit 07ba0a0 Author: Continuous Integration <dev@code.org> Date: Mon Oct 12 23:51:51 2015 +0000 Automatically built. commit d3ccd84 Author: Tanya Parker <tanyacparker@gmail.com> Date: Mon Oct 12 16:47:17 2015 -0700 Updated apps strings commit d3ccd84 Author: Tanya Parker <tanyacparker@gmail.com> Date: Mon Oct 12 16:47:17 2015 -0700 Updated apps strings commit 4346cb4 Merge: 2498416 e8a7d34 Author: Caley Brock <caleybrock7@gmail.com> Date: Mon Oct 12 16:37:25 2015 -0700 Merge pull request #4509 from code-dot-org/reviews Move review special events to teacher-dashboard

Fixes a number of regressions from #4464, including textarea padding and line height. The base.scss styles weren't getting applied in Design Mode anymore, leading to inconsistencies.

davidsbailey added 3 commits October 8, 2015 22:53

separate design mode content out of #divApplab into #designModeViz.

faf4803

clean up levelHtml init sequence

5cefd47

add a couple screens/levelHtml tests

4431521

davidsbailey assigned Bjvanminnen Oct 9, 2015

Bjvanminnen reviewed Oct 9, 2015
View reviewed changes

davidsbailey added 3 commits October 10, 2015 11:12

improve comments per code review feedback

54b7a2d

add 'design_' prefix to all children and grandchildren of designModeViz.

ca43099

fix tests after adding 'design_' prefix.

b4bf41e

davidsbailey added 3 commits October 12, 2015 14:11

code review feedback

6434a19

fix applabTest

46498ca

fix dimming regression. add tests.

7ff9cb2

davidsbailey added a commit that referenced this pull request Oct 13, 2015

Merge pull request #4464 from code-dot-org/design-mode-viz

5cb0308

separate design mode content out of divApplab

davidsbailey merged commit 5cb0308 into staging Oct 13, 2015

davidsbailey deleted the design-mode-viz branch October 13, 2015 14:21

davidsbailey mentioned this pull request Oct 14, 2015

Fix id dropdowns #4547

Merged

Navigation Menu

Uh oh!

separate design mode content out of divApplab - #4464

separate design mode content out of divApplab#4464
davidsbailey merged 9 commits into
stagingfrom
design-mode-viz

davidsbailey commented Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

Bjvanminnen Oct 9, 2015

Uh oh!

davidsbailey Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

davidsbailey commented Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

davidsbailey commented Oct 11, 2015

Uh oh!

davidsbailey commented Oct 11, 2015

Uh oh!

Bjvanminnen commented Oct 12, 2015

Uh oh!

davidsbailey commented Oct 13, 2015

Uh oh!

Bjvanminnen commented Oct 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

davidsbailey commented Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

Bjvanminnen Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

davidsbailey Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

davidsbailey commented Oct 9, 2015

Uh oh!

Bjvanminnen commented Oct 9, 2015

Uh oh!

davidsbailey commented Oct 11, 2015

Uh oh!

davidsbailey commented Oct 11, 2015

Uh oh!

Bjvanminnen commented Oct 12, 2015

Uh oh!

davidsbailey commented Oct 13, 2015

Uh oh!

Bjvanminnen commented Oct 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants