Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Update circle. by T4rk1n · Pull Request #270 · plotly/dash-core-components · GitHub
Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Update circle. - #270

Merged
T4rk1n merged 4 commits into
masterfrom
update-circle
Aug 31, 2018
Merged

Update circle.#270
T4rk1n merged 4 commits into
masterfrom
update-circle

Conversation

@T4rk1n

Copy link
Copy Markdown
Contributor

Update the config of circleci to version 2

  • Removed tox
  • Build with Builder was failing, added the configs of dash-component-boilerplate.
  • There is nothing but the tests to pylint, and they fail with really low score, so Pylint is out for now.

The test_link_scroll

defdisplay_page(pathname):
call_count.value=call_count.value+1
return'You are on page {}'.format(pathname)
self.startServer(app=app)
time.sleep(2)
#callback is called twice when defined
self.assertEqual(
call_count.value,
2
)
is somewhat brittle and sometimes fails.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@Marc-Andre-Rivet Could you perhaps review this if you have time? I think you know this stuff pretty well!

Comment thread.circleci/config.yml
version: 2

jobs:
"python-2.7": &test-template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not aware of this syntax '&test-template', you use it to refer to this job configuration for python36 and 37 so you can just override what you need?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I took that from plotly/dash#222.

Comment thread.circleci/config.yml Outdated
- run:
name: Run tests
command: |
npm run test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not important, but you can do 'npm test' instead

Comment threadpackage.json Outdated
"publish-pypi": "npm run prepublish && python setup.py sdist && twine upload --sign --skip-existing",
"start": "./node_modules/.bin/builder run build-dev",
"test": "./node_modules/.bin/eslint --fix .",
"test": "./node_modules/.bin/eslint --fix src",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find it weird that test is used to fix linting errors. Should probably be a script of its own like 'fixlint', 'lint.fix' or something along those lines.
You should be able to call 'eslint --fix src' directly instead too, same for builder above.

I would expect eslint to be in the list of dependencies if we are calling it in our scripts. We should not trust babel-eslint to pull it along.

Comment threadpackage.json
"author": "Chris Parmer <chris@plot.ly>",
"license": "MIT",
"dependencies": {
"babel-core": "^6.26.3",

@Marc-Andre-RivetMarc-Andre-RivetAug 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a shrinkwrap or package-lock file for this project? If we do not we should, as it makes the version of the dependencies downloaded by our users more predictable and saves us from weird compatibility issues that may arise from various technically-compatible but not identical versions.

Comment threadwebpack.config.js
}
]
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for this new webpack.config.js file? The webpack config stuff for the components is in ./config/webpack/, won't it be confusing to have 2 config files for webpack?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The old configs were for webpack 3, this is for webpack 4.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Maybe it's a good idea to update the .eslintrc file too while we're at it? In #274 I've used the settings taken from plotly/streambed, maybe we can do that here too? What do you think?

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman Yes we should add the prettier here, Its in the list plotly/dash#328

@valentijnnieman

valentijnnieman commented Aug 22, 2018

Copy link
Copy Markdown
Contributor

@T4rk1nCool! You could try merging in the #274 branch if you want! something went wrong with that branch, I pulled from it in another branch, deleted it, and merged that branch which caused #274 to be merged as well. My mistake!

@T4rk1n

Copy link
Copy Markdown
ContributorAuthor

@valentijnnieman I have added prettier and fixed the errors it was reporting, for the tabs file I put a eslint-disable at the top because it was reporting a bunch of prop types error for the EnhancedTab. Please review.

@valentijnnieman

Copy link
Copy Markdown
Contributor

@T4rk1n Oh yeah <EnhancedTab/> isn't using propTypes. I'll fix that later. Nice work! 💃

@chriddyp

Copy link
Copy Markdown
Member

💃

@chriddypchriddyp reopened this Aug 31, 2018
@T4rk1n
T4rk1n merged commit 557d2fd into masterAug 31, 2018
@T4rk1n
T4rk1n deleted the update-circle branch August 31, 2018 14:51
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@T4rk1n@valentijnnieman@chriddyp@Marc-Andre-Rivet