Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Update R package metadata YAML file for 1.0.2 #147

Merged
merged 24 commits into from
Jan 10, 2020
Merged

Update R package metadata YAML file for 1.0.2 #147

merged 24 commits into from
Jan 10, 2020

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Dec 18, 2019

The YAML data used for populating title/description/examples should be up to date in the dev branch. This PR will backcommit the updates.

@rpkyle
Copy link
Contributor Author

rpkyle commented Jan 8, 2020

@Marc-Andre-Rivet Could we merge this if there are no objections?

dash-info.yaml Outdated
app$layout(
htmlDiv(list(
htmlAcronym(children='Mouse over these words to see the acronym for \'as soon as possible\'.',
title='ASAP')
Copy link
Collaborator

Choose a reason for hiding this comment

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

htmlAcronym (and htmlAbbr above) should have their props reversed - children is the short text that's always shown, title is the longer explanation of it. https://www.w3docs.com/learn-html/html-acronym-tag.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in b4e683f

dash-info.yaml Outdated

app$layout(
htmlDiv(list(
htmlCaption("This is an example of htmlCaption.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in f462bce

htmlDetails(
children ="Hello"
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the htmlSummary example here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 03fabd8

)
)
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the htmlDt example here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 344fbfa

)
)
)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the htmlDt example here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in db36bfd

dash-info.yaml Outdated

app$layout(htmlDiv(list(
htmlFrameset(children =
htmlIframe(width = "600px", height = "600px",
Copy link
Collaborator

Choose a reason for hiding this comment

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

really? I'd think htmlFrameset should just show a deprecation comment like htmlFrame

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 19551a7

dash-info.yaml Outdated

app$layout(
htmlDiv(list(
htmlHeader(htmlH1("This is a header")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

a little confusing this is a heading (<h1>) inside a header (<header>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Literally taken straight from the MDN example 😆:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/header

Will remove the heading tag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's common to put headings in headers, just the way it's shown it makes it look like you're saying the <h1> is the header. Anyway yeah, cleaner for an example to just do htmlHeader("This is a header")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 64a6b68

rpkyle and others added 2 commits January 9, 2020 17:16
dash-info.yaml Outdated
app$layout(
htmlDiv(list(
htmlIsindex(prompt = 'Search Document..')
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should have a deprecation comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 3ed2ec5

htmlDiv(list(
htmlImg(src = "https://upload.wikimedia.org/wikipedia/commons/0/0c/PIA17351-ApparentSizes-MarsDeimosPhobos-EarthMoon.jpg",
useMap = "#image-map"),
htmlMapEl(list(
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this the only one that has a different name from its HTML counterpart (<map>)? we should add a comment about this (and to any others if there are any) so users can find regular HTML documentation about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 5caaeda

dash-info.yaml Outdated
- name: htmlNextid
dontrun: TRUE
code: |
# Warning: The <nextid> tag is obsolete, as of HTML Version 3.2.
Copy link
Collaborator

Choose a reason for hiding this comment

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

then take out the code below, which doesn't use htmlNextid at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 5555dff

dash-info.yaml Outdated
htmlH2("Warning: A browser without support for JavaScript
will show the text inside the noscript element.
Since Dash uses JavaScript internally,
using this option might cause incompatibility issues."),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt it... probably just wouldn't show up at all. Anyway since it would need js in order to create it, the only reasonable option is to put <noscript> in the index template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't write this example, so I'm not sure. But that makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 46a624c

)

app$run_server()
- name: htmlObjectEl
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah this one also has a mutated name...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 989ac96


app$run_server()
- name: htmlTfoot
do
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we not wrap the strings in htmlP, just "longstring", htmlWbr(), "secondpart"? I don't think it does anything as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This alternative:

app$layout(
  htmlDiv(list(
    "In a long string, it might be a good idea to add an htmlWbr to specify word breaks",
    "Thisverylongstringwithnowhitespaceswon'tlookverygood",
    htmlWbr(),
    "butatleastyoucanspecifya'natural'placeforthestringtobebrokenup"
  )
 )
)

yields

In a long string, it might be a good idea to add an htmlWbr to specify word breaksThisverylongstringwithnowhitespaceswon'tlookverygoodbutatleastyoucanspecifya'natural'placeforthestringtobebrokenup

while the original snippet appears to produce

In a long string, it might be a good idea to add an htmlWbr to specify word breaks
Thisverylongstringwithnowhitespaceswon'tlookverygood

butatleastyoucanspecifya'natural'placeforthestringtobebrokenup

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm not worth spending a lot of time on, but if you're already breaking the words into different <p> then what does it matter if you <wbr>? Seems like then it just acts as a <br>. But if I try it in Python using just strings or strings in <span>s:

html.Div([
    "a" * 100,
    "b" * 100,
    html.Wbr(),
    "c" * 100,
    html.Wbr(),
    html.Span("d" * 100),
    html.Span("e" * 100)
])

I get what I expected:
Screen Shot 2020-01-09 at 9 21 09 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, will merge this now but revisit this example later.

app$layout(htmlDiv(list(
htmlForm(children=list(
htmlP(children=list('Username: ', dccInput(type='text', id='username', placeholder='username'))),
htmlP(children=list('Password: ', dccInput(type='password', id='password', placeholder='password'))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

you need library(dashCoreComponents) for this example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 209a10d

code: |
library(dash)
library(dashHtmlComponents)
library(dashCoreComponents)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally though I suppose we don't want library(dashCoreComponents) where it's not used... that happens a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, and I'm guilty of the same. It was sloppy to not purge those entries from the examples. We can't always live in the ideal world, but it's better to lead by (good) example.

fixed in 137542f

@alexcjohnson
Copy link
Collaborator

As discussed on slack, I gave this a more thorough review largely so we can at some point use these code snippets automatically in the dashr.plot.ly docs for the html components.

@rpkyle rpkyle requested a review from alexcjohnson January 10, 2020 01:44
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 thanks for the tweaks!

@rpkyle rpkyle merged commit f879970 into dev Jan 10, 2020
@rpkyle rpkyle deleted the update-r-yaml branch January 10, 2020 04:40
Sign up for free to 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.

3 participants