-
Notifications
You must be signed in to change notification settings - Fork 949
(Partially) remove jQuery and Bootstrap #427
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
(Partially) remove jQuery and Bootstrap #427
Conversation
👍 thanks for cleaning up the PR ! |
model.get('_view_name'), | ||
model.get('_view_module'), | ||
ManagerBase._view_types | ||
).then(function(ViewType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, more legible, thanks!
this.label = document.createElement('div'); | ||
this.label.classList.add('widget-label'); | ||
this.label.style.display = 'none'; | ||
this.el.appendChild(this.label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing HTML in JS is so awkward (same with the jquery version). This is one of those moments I wish we were using JSX or webpack to pull in the HTML. Maybe in a separate PR we should look into webpacking HTML...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely more verbose. The way we mostly deal with it in phosphor
is by having a small number of static
methods, like createNode
, etc. and just having those be pure DOM construction. It ends up being mostly okay at that point. It's not as small as JSX, but it's totally readable and predictable.
this.droplist.style.maxHeight = availableHeightAbove + 'px'; | ||
return; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic!
var radios = _.pluck( | ||
this.container.querySelectorAll('input[type="radio"]'), | ||
'value' | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool trick!
(Partially) remove jQuery and Bootstrap
Sweet, thanks! |
The heroic work came from @afshin, thanks dude! |
FYI, @jdfreder @SylvainCorlay @jasongrout @dwillmer ... this is to (soon) supersede #400