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
Choose a base branch
from
Open

$mol_view added #135

wants to merge 3 commits into from

Conversation

nin-jin
Copy link

@nin-jin nin-jin commented Mar 17, 2019

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

'$mol_view': {
Copy link
Owner

Choose a reason for hiding this comment

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

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

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

Copy link
Author

Choose a reason for hiding this comment

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

$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) {
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

@nin-jin nin-jin Jun 29, 2019

Choose a reason for hiding this comment

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

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

@johnmichel johnmichel self-assigned this Jun 29, 2019
@nin-jin
Copy link
Author

nin-jin commented Apr 4, 2021

Why not accept it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants