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

Markdown dedent, and remove containerProps #569

Merged
merged 9 commits into from
Jun 14, 2019
Merged

Markdown dedent, and remove containerProps #569

merged 9 commits into from
Jun 14, 2019

Conversation

alexcjohnson
Copy link
Collaborator

Closes #567

Breaking change for Dash 1.0

Added a dedent prop to the Markdown component, set to true by default. Behavior is intended to match textwrap.dedent - in particular:

  • Empty lines are ignored
  • Lines with only whitespace are emptied, then ignored
  • All other lines: we find the longest leading whitespace string they all share, and remove it from them all.
  • Spaces and tabs are both considered whitespace, but are not considered equal.

Also:

  • Removed the containerProps prop - based on the refactor @wbrgss did in Native syntax highlighting in dcc.Markdown #562 it appears to be useless, as we have the necessary id, className, and style on the container already.
  • Stopped passing on extra props to the underlying react-markdown component. All props are already used explicitly, this seems like it can only cause problems, it's probably a relic of some older usage.

@alexcjohnson alexcjohnson requested a review from wbrgss June 14, 2019 01:27
@@ -11,6 +11,11 @@ import './css/highlight.css';
* [react-markdown](https://rexxars.github.io/react-markdown/) under the hood.
*/
class DashMarkdown extends Component {
constructor(props) {
super(props);
this.highlightCode = this.highlightCode.bind(this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

apparently not needed given our current usage, but makes me nervous to have it there unbound...

pre +
' b\r' +
pre +
'c\r\n' +
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Prettier certainly did not improve clarity here... and its indentation choice is peculiar for this case. Oh well...

Copy link
Contributor

Choose a reason for hiding this comment

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

cannot partially disable just for this chunk?

const text = ' a\n b';
const mdDedented = render(<Markdown children={text} />);
expect(mdDedented.find('code').length).toEqual(0);
expect(mdDedented.find('p').length).toEqual(1);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dedented: makes everything into a <p> element.


const mdRaw = render(<Markdown children={text} dedent={false} />);
expect(mdRaw.find('code').length).toEqual(1);
expect(mdRaw.find('p').length).toEqual(0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dedent turned off: the leading spaced mean it gets turned into a code block.

@alexcjohnson
Copy link
Collaborator Author

@wbrgss Two things I missed when reviewing recent PRs:

  • most of integration tests got commented out! That's why Percy is complaining it got no snapshots 🤦‍♂ fixed in d207da3
  • changelog for the syntax highlighter removal PR, along with this PR df5192d

@alexcjohnson
Copy link
Collaborator Author

Hmm, the percy diffs, now that they're back, may point to a problem, inline code is not actually inline...

Copy link
Contributor

@wbrgss wbrgss left a comment

Choose a reason for hiding this comment

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

RegEx looks good to me. I'll double-check by using this in the docs (replacing SyntaxHighlighter) to see if there are any regressions.

@@ -4,6 +4,21 @@ This project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]
### Added
- `Markdown` components support code highlighting - no need to switch to `SyntaxHighlighter`, which has been removed. Use triple backticks, with the opening backticks followed by the language name or abbreviation. [#562](https://github.com/plotly/dash-core-components/pull/562) Supported languages:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add that Markdown will attempt to auto-detect the language (via highlight.js) without specifying the language name

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe mention the theme prop, but this could be in the docs for dcc.Markdown as well/instead

# for _ in self.driver.find_element_by_css_selector(
# '#date-picker-range-output').text.split(' - ')])),
# date_tokens,
# "date should match the callback ou
Copy link
Contributor

Choose a reason for hiding this comment

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

🙈 👍

const commonLen = commonPrefix ? commonPrefix.length : 0;
return lines
.map(line => {
return line.match(/\S/) ? line.substr(commonLen) : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@wbrgss wbrgss self-requested a review June 14, 2019 19:13
Copy link
Contributor

@wbrgss wbrgss left a comment

Choose a reason for hiding this comment

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

💃
Cool, this should make our doc-writing lives easier 🎊

Changelog additions aren't blocking for me

@byronz byronz merged commit aca7283 into master Jun 14, 2019
@byronz byronz deleted the markdown-dedent branch June 14, 2019 19:25
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.

For 1.0, dedent in Markdown
3 participants