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.

Sign up for GitHub

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

Enable server-side/isomorphic rendering #360

Merged
merged 11 commits into from
Dec 13, 2016
Merged

Enable server-side/isomorphic rendering #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e9c89e0
Conditionally load DOM4 polyfill (load only in the browser)
codebling Dec 13, 2016
f281672
Check for presence of 'document' before attempting to use it. Safely …
codebling Dec 13, 2016
32dda35
Check for presence of 'navigator' before attempting to use it. Safely…
codebling Dec 13, 2016
345e1fa
Store the "show dialog" key rather than the key combo in a const. Fix…
codebling Dec 13, 2016
69568c3
Check that 'document.documentElement.clientWidth' exists and is a num…
codebling Dec 13, 2016
f77bd1a
Move event bindings to 'componentDidMount', which fixes server-side r…
codebling Dec 13, 2016
dd490e9
Return an void/noop Manager in a server context. Makes FocusStyleMana…
codebling Dec 13, 2016
828acbe
Fix lint
codebling Dec 13, 2016
df12c62
Don't calculate code-coverage for this file, since functionality is s…
codebling Dec 13, 2016
6a53b13
Remove useless tslint hint
codebling Dec 13, 2016
97c0800
Ignore coverage checks on code that can't be meaningfully tested.
codebling Dec 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"typings": "dist/index.d.ts",
"style": "dist/blueprint.css",
"dependencies": {
"@types/dom4": "^1.5.20",
"classnames": "^2.2",
"dom4": "^1.8",
"normalize.css": "4.1.1",
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/accessibility/focusStyleManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,17 @@ import { InteractionModeEngine } from "../common/interactionMode";

export const FOCUS_DISABLED_CLASS = "pt-focus-disabled";

const focusEngine = new InteractionModeEngine(document.documentElement, FOCUS_DISABLED_CLASS);
/* istanbul ignore next */
const fakeFocusEngine = {
isActive: () => true,
start: () => true,
stop: () => true,
};

/* istanbul ignore next */
const focusEngine = typeof document !== "undefined"
? new InteractionModeEngine(document.documentElement, FOCUS_DISABLED_CLASS)
: fakeFocusEngine;

// this is basically meaningless to unit test; it requires manual UI testing
/* istanbul ignore next */
Copy link
Contributor

Choose a reason for hiding this comment

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

Unit test coverage is failing for this file; it's pretty hard to cover this functionality anyway, so we should disable the whole file. I suggest moving this /* istanbul ignore next */ line to the top, right after the license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks. I was just struggling with that. Coverage inexplicably runs on CircleCI but not locally, and I can't figure out why.

Copy link
Contributor

@adidahiya adidahiya Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Have you tried gulp karma-unit-core gulp karma-core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been using gulp test, which is an alias for gulp karma. gulp karma-core seems to do the same thing but only for the 'core' package. In all cases a coverage report is generated, but it is empty (shows 100% Statements 0/0 100% Branches 0/0 100% Functions 0/0 100% Lines 0/0). Anyhow, coverage seems to pass now.

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/components/hotkeys/hotkeyParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export const Aliases = {
command: "meta",
escape: "esc",
minus: "-",
mod: /Mac|iPod|iPhone|iPad/.test(navigator.platform) ? "meta" : "ctrl",
mod: ((typeof navigator !== "undefined") && /Mac|iPod|iPhone|iPad/.test(navigator.platform)) ? "meta" : "ctrl",
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like we should have an isUndefined utility next to isFunction

option: "alt",
plus: "+",
return: "enter",
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/components/hotkeys/hotkeysEvents.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { comboMatches, getKeyCombo, IKeyCombo, parseKeyCombo } from "./hotkeyPar
import { IHotkeysProps } from "./hotkeys";
import { isHotkeysDialogShowing, showHotkeysDialog } from "./hotkeysDialog";

const SHOW_DIALOG_KEY_COMBO = parseKeyCombo("?");
const SHOW_DIALOG_KEY = "?";
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


export enum HotkeyScope {
LOCAL,
Expand Down Expand Up @@ -59,7 +59,7 @@ export class HotkeysEvents {

const combo = getKeyCombo(e);

if (comboMatches(SHOW_DIALOG_KEY_COMBO, combo)) {
if (comboMatches(parseKeyCombo(SHOW_DIALOG_KEY), combo)) {
showHotkeysDialog(this.actions.map((action) => action.props));
return;
}
Expand Down
18 changes: 15 additions & 3 deletions packages/core/src/components/hotkeys/hotkeysTarget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@ export interface IHotkeysTarget extends React.Component<any, any>, React.Compone
}

export function HotkeysTarget<T extends { prototype: IHotkeysTarget }>(constructor: T) {
const { componentWillMount, componentWillUnmount, render, renderHotkeys } = constructor.prototype;
const {
componentWillMount,
componentDidMount,
componentWillUnmount,
render,
renderHotkeys,
} = constructor.prototype;

if (!isFunction(renderHotkeys)) {
throw new Error(`@HotkeysTarget-decorated class must implement \`renderHotkeys\`. ${constructor}`);
Expand All @@ -37,12 +43,18 @@ export function HotkeysTarget<T extends { prototype: IHotkeysTarget }>(construct
this.localHotkeysEvents = new HotkeysEvents(HotkeyScope.LOCAL);
this.globalHotkeysEvents = new HotkeysEvents(HotkeyScope.GLOBAL);

if (componentWillMount != null) {
componentWillMount.call(this);
}
};

constructor.prototype.componentDidMount = function() {
// attach global key event listeners
document.addEventListener("keydown", this.globalHotkeysEvents.handleKeyDown);
document.addEventListener("keyup", this.globalHotkeysEvents.handleKeyUp);

if (componentWillMount != null) {
componentWillMount.call(this);
if (componentDidMount != null) {
componentDidMount.call(this);
}
};

Expand Down
7 changes: 6 additions & 1 deletion packages/core/src/components/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
* and https://github.com/palantir/blueprint/blob/master/PATENTS
*/

import "dom4";
declare function require(moduleName: string): any; // declare node.js "require" so that we can conditionally import
Copy link
Contributor

Choose a reason for hiding this comment

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

note for a follow-up change (doesn't have to be in this PR): we should share this ambient declaration with https://github.com/palantir/blueprint/blob/497a03eab4420e48db0865407995895aa25726fa/packages/docs/src/require-shim.d.ts

Copy link
Contributor Author

@codebling codebling Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I included it there in the same file specifically because I was worried about scoping. It makes sense to merge them. Personally, I find the Node.js form clearer than the one used in require-shim, but whatever works!

if (typeof window !== "undefined" && typeof document !== "undefined") { // we're in browser
// tslint:disable-next-line:no-var-requires
require("dom4"); // only import actual dom4 if we're in the browser (not server-compatible)
// we'll still need dom4 types for the TypeScript to compile, these are included in package.json
}

import * as contextMenu from "./context-menu/contextMenu";
export const ContextMenu = contextMenu;
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/components/menu/menuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,13 @@ export class MenuItem extends AbstractComponent<IMenuItemProps, IMenuItemState>
}

let { left = 0, right = 0 } = this.props.submenuViewportMargin;
right = document.documentElement.clientWidth - right;
if (typeof document !== "undefined"
&& typeof document.documentElement !== "undefined"
&& Number(document.documentElement.clientWidth)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

implicit coercion number => boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I spotted this, but unfortunately we can't test for NaN because NaN == NaN is never true. Number.isNaN() would be perfect, but only works if you're targeting es6. Otherwise, NaN correctly coerces to false, and the only other problematic value (coerces to a value we don't want) is 0, but if the clientWidth is 0 then it doesn't matter what the alignment is. So I left as-is. Only other option would be check if it's defined.

// we're in a browser context and the clientWidth is available,
// use it to set calculate 'right'
right = document.documentElement.clientWidth - right;
}
// uses context to prioritize the previous positioning
let alignLeft = this.context.alignLeft || false;
if (alignLeft) {
Expand Down