Initial Quick Docs implementation #3449

peterflynn · 2013-04-16T05:51:04Z

This introduces:

In core: a "Quick Docs" command and an extension point for it, paralleling Quick Edit
In an extension: a specific Quick Docs provider for the Web Platform Docs CSS dataset

This doesn't include final XD layout/styles yet, so initial feedback should focus on the JS parts.

…arallel to Quick Edit; and Web Platform Docs provider for that extension point, based on original prototype at https://github.com/awgreenblatt/InlineWPD.

* origin/master: (28 commits) updated package validator based on comments in #3437 remove spurious IntelliJ files Updated by ALF automation. Another round of code review changes. Move HTML templates to htmlContent Tweaked dialog so it's centered a little better Renamed ExtensionMgr to ExtensionManager Code review fixes. Update comments to remove the Mustache note, since these can work with Handlebars too Make JSHint happy. Respond to review comments Initial extension manager dialog/view/model (with non-final layout) Move out hover preview menu item label to strings.js for localization. Remove unused variables and clean up some variable declarations. Merging Glenn's Hover Preview extension into core. Only allow leading numbers and letters (largely to prevent names like "..") import rewire to assist in testing Update Getting started screenshot for 'de' locale Fix DISALLOWED_WORDS placeholder in 'de' locale Updated by ALF automation. 'de' locale: Fix Travis build errors (duplicate strings), minor wording edits ...

jasonsanjose · 2013-04-16T23:03:48Z

Reviewing

jasonsanjose · 2013-04-16T23:08:11Z

src/command/DefaultMenus.js

@@ -130,6 +130,8 @@ define(function (require, exports, module) {
        menu.addMenuItem(Commands.TOGGLE_QUICK_EDIT);
        menu.addMenuItem(Commands.QUICK_EDIT_PREV_MATCH);
        menu.addMenuItem(Commands.QUICK_EDIT_NEXT_MATCH);
+        menu.addMenuDivider();


Normally we ask extensions to add menus within the extension and not in DefaultMenus right?

Right, but this isn't an extension Command, is a new Quick Docs Command handled by the Editor Manager and the extension just adds a new Quick Docs provider that uses this command, just like Quick Edit works.

Ah, yes. Thanks for catching my mistake. :)

jasonsanjose · 2013-04-17T00:10:13Z

Initial review complete

* Simplify InlineDocsViewer by not overriding load()

peterflynn · 2013-04-17T18:01:20Z

@jasonsanjose the final UI parts have landed if you'd like to get started reviewing the HTML/LESS while I finish making those changes from your JS feedback.

* Update inline widget height when horizontal resizing causes word wrap to shift around * Show full URL on link tooltips * Remove unneeded timeout in onAdded() which was copied from InlineImageViewer (where it seems to just be a hack for image load timing) * Remove unneeded check for spaces in URLs * Fix missing 'return null'

peterflynn · 2013-04-17T23:29:45Z

@jasonsanjose fixes pushed. The two issues within the JSON file (uppercase properties and forward slash escaping) are still waiting on Adam, so would you mind spinning them off as a code cleanup bug to unblock the pull request?

jasonsanjose · 2013-04-17T23:46:48Z

Sure, see #3471. Reviewing changes.

jasonsanjose · 2013-04-18T00:06:53Z

src/extensions/default/WebPlatformDocs/main.js

+            return result.promise();
+
+        } else {
+            return null;


Nit: I was thinking more of a base case return null outside of this if/else block, at the end of this function.

I thought this way would help emphasize that the if above doesn't have any fall-through cases...

jasonsanjose · 2013-04-18T00:14:03Z

Nested scrolling in an inline widget is just as bad as I remembered it. I think it's ok to take for now, but I think it warrants more discussion.

jasonsanjose · 2013-04-18T02:38:44Z

src/extensions/default/WebPlatformDocs/InlineDocsViewer.js

+
+        // Set height initially, and again whenever width might have changed (word wrap)
+        this._sizeEditorToContent();
+        $(window).on("resize", this._sizeEditorToContent);


We should resize when the sidebar shows/hides too. We can file that separately if you like.

Added this cleanup to #3471

Thanks, good catch. I'll assign the bug to myself.

jasonsanjose · 2013-04-18T02:53:52Z

src/extensions/default/WebPlatformDocs/InlineDocsViewer.html

+        <h1>{{propName}}</h1>
+        <div>{{{summary}}}</div>
+    </div>
+    <div class="divider-holder">


Usually I'm not picky about non-semantic markup, but this jumped out at me. I assume this is tricky for some reason?

Yeah... see the comment on the LESS rule for it. Essentially, to get the line centered in the gap between columns we need its offset to be some percentage plus half the fixed-size gap. I suppose we could use calc() for that but I've always been a little unsure how well non-IE browsers support that stuff...

Fwiw the .content-bottom div is another thing we only need for layout, since it's impossible to put a margin/padding gap directly between floated content and an immediate sibling (e.g. see http://stackoverflow.com/questions/4198269/in-css-margin-top-is-not-working-with-clear-both).

jasonsanjose · 2013-04-18T03:03:12Z

@peterflynn done with round 2. I think the only thing blocking an immediate merge is the quiet-scrollbars issue. We can file everything else as cleanup. I would have made the change myself and merged, but in just in case, I want to give you the chance to make the call.

peterflynn · 2013-04-18T04:18:39Z

Re the nested scrolling UX: yeah... I think we should do something to improve it a bit before sprint's end. What do you think of the three changes I proposed on email yesterday? (bottom shadow; make scrollbar visible while using arrow keys; don't let scrollwheel/trackpad "overrun" and start scrolling editor).

For me the biggest pain point is the scrollwheel issue... but once the keyboard nav work is in maybe that will become more annoying for many people... hard to say.

links; remove redundant bind() call

peterflynn · 2013-04-18T05:06:52Z

Pushed up another round of fixes

jasonsanjose · 2013-04-18T16:33:47Z

re: nested scrolling

Yes to a below-the-fold indicator
Show scrollbars on keyboard nav...mice to have, but lower priority to me than the shadow and the janky nested scroll.
Yes to prevent bubbling. Agreed that this is the biggest annoyance.

jasonsanjose · 2013-04-18T17:41:08Z

Merging

Initial Quick Docs implementation

peterflynn added 2 commits April 15, 2013 18:12

Introduce "Quick Docs" command and extensibility point, similar but p…

058275f

…arallel to Quick Edit; and Web Platform Docs provider for that extension point, based on original prototype at https://github.com/awgreenblatt/InlineWPD.

Change Quick Docs shortcut after discussion with Adam

fe83c92

ghost assigned jasonsanjose Apr 16, 2013

jasonsanjose reviewed Apr 16, 2013
View reviewed changes

peterflynn added 2 commits April 17, 2013 00:38

* XD design for Quick Docs, including quiet scrollbars

e40867c

* Simplify InlineDocsViewer by not overriding load()

Add license info for Web Platform Docs content & logo

0f5caab

jasonsanjose mentioned this pull request Apr 17, 2013

[PM] Cleanup WebPlatformDocs css.json #3471

Closed

jasonsanjose reviewed Apr 18, 2013
View reviewed changes

Code review: use LESS vars for text colors; ensure URL tooltip on *all*

01e5c56

links; remove redundant bind() call

jasonsanjose added a commit that referenced this pull request Apr 18, 2013

Merge pull request #3449 from adobe/pflynn/quick-docs-api

d8d5a15

Initial Quick Docs implementation

jasonsanjose merged commit d8d5a15 into master Apr 18, 2013

jasonsanjose deleted the pflynn/quick-docs-api branch April 18, 2013 17:41

core-ai-bot mentioned this pull request Aug 29, 2021

[CLOSED] Initial Quick Docs implementation brackets-archive/bracketsIssues#3239

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Quick Docs implementation #3449

Initial Quick Docs implementation #3449

peterflynn commented Apr 16, 2013

jasonsanjose commented Apr 16, 2013

jasonsanjose Apr 16, 2013

TomMalbran Apr 16, 2013

jasonsanjose Apr 17, 2013

jasonsanjose commented Apr 17, 2013

peterflynn commented Apr 17, 2013

peterflynn commented Apr 17, 2013

jasonsanjose commented Apr 17, 2013

jasonsanjose Apr 18, 2013

peterflynn Apr 18, 2013

jasonsanjose commented Apr 18, 2013

jasonsanjose Apr 18, 2013

jasonsanjose Apr 18, 2013

peterflynn Apr 18, 2013

jasonsanjose Apr 18, 2013

peterflynn Apr 18, 2013

jasonsanjose commented Apr 18, 2013

peterflynn commented Apr 18, 2013

peterflynn commented Apr 18, 2013

jasonsanjose commented Apr 18, 2013

jasonsanjose commented Apr 18, 2013

Initial Quick Docs implementation #3449

Initial Quick Docs implementation #3449

Conversation

peterflynn commented Apr 16, 2013

jasonsanjose commented Apr 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Apr 17, 2013

peterflynn commented Apr 17, 2013

peterflynn commented Apr 17, 2013

jasonsanjose commented Apr 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Apr 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonsanjose commented Apr 18, 2013

peterflynn commented Apr 18, 2013

peterflynn commented Apr 18, 2013

jasonsanjose commented Apr 18, 2013

jasonsanjose commented Apr 18, 2013