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

$mol_view added #135

Open
wants to merge 3 commits into
base: master
from

Conversation

@nin-jin
Copy link

commented Mar 17, 2019

nin-jin added some commits Mar 17, 2019

@@ -437,6 +437,20 @@ var d41d8cd98f00b204e9800998ecf8427e_LibraryDetectorTests = {
}
},

'$mol_view': {

This comment has been minimized.

Copy link
@johnmichel

johnmichel Jun 29, 2019

Owner
Suggested change
'$mol_view': {
'$mol': {

Since the documentation for this library refers to itself as $mol, the name should reflect that.

This comment has been minimized.

Copy link
@nin-jin

nin-jin Jun 29, 2019

Author

$mol is the namespace. There are many libraries. $mol_view (view components base class) is one of them.

icon: 'mol',
url: 'http://mol.js.org/',
test: function(win) {
if (win.$mol_view) {

This comment has been minimized.

Copy link
@johnmichel

johnmichel Jun 29, 2019

Owner
Suggested change
if (win.$mol_view) {
if (win.$mol_view || win.document.querySelector('$mol_view_root')) {

Would be preferable to having two, single-condition if blocks here.

This comment has been minimized.

Copy link
@nin-jin

nin-jin Jun 29, 2019

Author

I think 2 simplest conditions are easier to understand than 1 complex condition.

@johnmichel johnmichel self-assigned this Jun 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.