Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rpkyle@josegonzalez@Marc-Andre-Rivet@byronz@HammadTheOne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rpkyle@josegonzalez@Marc-Andre-Rivet@byronz@HammadTheOne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rpkyle@josegonzalez@Marc-Andre-Rivet@byronz@HammadTheOne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rpkyle@josegonzalez@Marc-Andre-Rivet@byronz@HammadTheOne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rpkyle@josegonzalez@Marc-Andre-Rivet@byronz@HammadTheOne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

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

Dash for R v0.5.0 - #205

Merged
rpkyle merged 113 commits into
masterfrom
dev
May 29, 2020
Merged

Dash for R v0.5.0#205
rpkyle merged 113 commits into
masterfrom
dev

Conversation

@rpkyle

@rpkylerpkyle commented May 28, 2020

Copy link
Copy Markdown
Contributor

[0.5.0 ] - 2020-05-28

Added

  • Dash for R now depends on the brotli package explicitly; previously it was loaded when importing reqres. #204

Changed

  • Dash for R no longer wraps the layout in an htmlDiv internally, for parity with Dash for Python. Starting in v0.5.0, the layout method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. #121
  • Package documentation has been significantly refactored to use new features of roxygen2 when documenting R6 classes
  • The title method now specifies Dash as the default application title instead of dash. #200

Fixed

  • A minor bug in validate_keys which prevented interpolate_index from working as intended has been resolved

rpkyleand others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors
* rename pruned to prune
* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors
* rename pruned to prune
* 🔨 handle stop errors
* 🚨 add test for stop errors
byron
add more trace for pytest
* Add line number context to stack traces when srcrefs are available (#133)
* ✨ Support line #s when in debug mode
* ✨ Add use_viewer option for RStudio
* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags
* support arbitrary tags
* 🚨 add tests
* 🔬 add asserts
* add reference to meta tag PR
* ⏩ indent meta tags
* add eager_loading parameter
* 📛 add buildFingerprint
* 📛 add checkFingerprint
* use getDependencyPath, + 🐾/Etag support
* updates to support async
* ✨ properly support gz compression
* 🐛 post-async fixes for CSS handling
…races (#137)
* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces
* 🚚 add polyfill.js
* refactor resolvePrefix
* camel case resolve_prefix
* Update R/utils.R
Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>

@josegonzalezjosegonzalez left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just come general feedback.

Comment threadCHANGELOG.md
- Dash for R now depends on the `brotli` package explicitly; previously it was loaded when importing `reqres`. [#204](https://github.com/plotly/dashR/pull/204)

### Changed
- Dash for R no longer wraps the layout in an `htmlDiv` internally, for parity with Dash for Python. Starting in v0.5.0, the `layout` method only accepts a single argument, and that argument must be a Dash component or a function that returns a Dash component. [#121](https://github.com/plotly/dashR/pull/121)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this mean for the majority of apps? Any changes folks will need to do to make sure their apps don't do something odd?

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.

@josegonzalez Good question; ultimately, it means that code like this:

app$layout(
htmlDiv("Some text"),
htmlDiv("Some more text")
)

is no longer treated as valid. It now needs to be wrapped within a div, with the children passed as a list:

app$layout(
htmlDiv(
list(
htmlDiv("Some text"),
htmlDiv("Some more text")
)
)
)

This was an artifact of the initial implementation (from the very, very beginning of Dash for R), which supported passing components as ... (like the R equivalent of kwargs). Realistically it's not supported by Dash, but was possible in Dash for R previously.

I'd consider this a breaking change-level modification, but now is the time to 🔪 the offending code, before we reach v1.0.

Comment thread.circleci/config.yml
echo "RUNNING JOB: ${CIRCLE_JOB}"
echo "JOB PARALLELISM: ${CIRCLE_NODE_TOTAL}"
echo "CIRCLE_REPOSITORY_URL: ${CIRCLE_REPOSITORY_URL}"
export PERCY_ENABLE=0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add this as an enhancement :D

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.

😹 I had forgotten to restore Percy after disabling it on a day where the tests were behaving strangely, so this was basically done in passing.

Comment threadR/utils.R
ids <- lapply(names(map), function(x) dash:::getIdProps(x)$ids)
props <- lapply(names(map), function(x) dash:::getIdProps(x)$props)
ids <- lapply(names(map), function(x) getIdProps(x)$ids)
props <- lapply(names(map), function(x) getIdProps(x)$props)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why the namespace removal?

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.

@josegonzalez The ::: syntax is useful for accessing functions in other packages which are not exported. Usually it's something you'd apply interactively, and not a technique to apply within R package code.

What's particularly strange is that this was added to access a function within the same package, no idea why I or anyone else did this, but in any event the CRAN check is careful to disallow this behaviour for submitted packages (and with good reason).

Very much a 🙈 moment.

Comment threadR/utils.R
}

validate_keys <- function(string) {
validate_keys <- function(string, is_template) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Does the signature change impact existing calls to validate_keys that only have a single argument?

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.

@josegonzalezvalidate_keys is an internal function, and only called from two Dash methods:

  • interpolate_index
  • index_string

The change makes it possible to use the same function for both of those methods, which pass different arguments to validate_keys (one passes the page index as a string with interpolation keys included, e.g. {%app_entry%} while the other passes in a vector of strings, in which the names of the keys are included, e.g. app_entry.

Both are matched against a list of necessary keys, and a useful error message thrown if the required keys are not present.

)
})

test_that("Customizing title using `name` produces a warning", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we raise an error now instead?

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.

@josegonzalez Good question. We previously raised an error in the last release, but this method was deprecated in that version, and now removed in this one.

@rpkyle
rpkyle merged commit c0c2563 into masterMay 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rpkyle@josegonzalez@Marc-Andre-Rivet@byronz@HammadTheOne