Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

RDMD Next #632

Merged
merged 57 commits into from
Apr 28, 2020
Merged

RDMD Next #632

merged 57 commits into from
Apr 28, 2020

Conversation

rafegoldberg
Copy link
Contributor

@rafegoldberg rafegoldberg commented Apr 27, 2020

Demo App ReadMe PR

This just pulls in a bunch of commits from the RDMD project, with a few additional fixes here and there.

@rafegoldberg rafegoldberg mentioned this pull request Apr 27, 2020
7 tasks
account for CSS var-based themes
Occasionally, authors do not add the proper line breaks between markdown blocks. Because our code tab regex was dependent on certain newline patterns, this would render some wonky shit. I cleaned up the pattern so it's now recognized entirely based on concurrent backticks! wahoo!
since this adds extra markup to acheive _such_ a simple, CSS-driven solution, we need to make sure to test this against the pertinent projects in the paper doc
dont add light box to custom emoji images
account for additional lightbox markup
lol i was wrong… we still need to check the `className` prop for the lang
class names now follow the data structure of the project.appearance.rdmd options object
@rafegoldberg rafegoldberg requested a review from erunion April 27, 2020 20:13
Comment on lines +193 to +199
<NewVariablesContext.Provider key={index} value={this.props.variables}>
<OauthContext.Provider value={this.props.oauth}>
<GlossaryTermsContext.Provider value={this.props.glossaryTerms}>
<NewGlossaryTermsContext.Provider value={this.props.glossaryTerms}>
<BaseUrlContext.Provider value={this.props.baseUrl.replace(/\/$/, '')}>
<NewBaseUrlContext.Provider value={this.props.baseUrl.replace(/\/$/, '')}>
<SelectedAppContext.Provider value={this.state.selectedApp}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erunion per some of my comments on this issue, I've double-wrapped the explorer's doc contents in both the old and new context providers. This is super verbose/redundant solution, but an interim one. I just haven't had a chance to go through and switch out all the references.

Copy link
Member

@erunion erunion left a comment

Choose a reason for hiding this comment

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

minor nits but otherwise lgtm

c: 'cplusplus',
'c++': 'cplusplus',
cpp: 'cplusplus',
docker: 'dockerfile',
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add this to syntax-highlighter/modes.js and add a test for it to assert that docker returns the same result as dockerfile?

And since I added support for a bunch of other aliases, I'm not sure if you just want to make a new remap file in the syntax-highlighter package, or pull those in here, or what.

packages/markdown/components/Code/index.jsx Outdated Show resolved Hide resolved
packages/markdown/components/Code/index.jsx Outdated Show resolved Hide resolved
packages/markdown/components/Image/index.jsx Show resolved Hide resolved
@@ -0,0 +1,17 @@
const flatMap = require('unist-util-flatmap');

// Adds an empty <div id='section-slug'> next to all headings.
Copy link
Member

Choose a reason for hiding this comment

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

Curious, but why not apply that id to the headings themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ fidlar dude

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By which I mean, obviously, that is a good suggestion.

Copy link
Contributor Author

@rafegoldberg rafegoldberg Apr 28, 2020

Choose a reason for hiding this comment

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

We could (at some point) slap the prefixed ID on the heading element itself, instead of adding it in a separate div, and leave the unprefixed one as the literal anchor icon! But Dom did this work, so not sure how difficult the HAST modification is for this. Looked pretty simple from a quick review, but.

code tabs are now delimited by an EOL char or two trailing linebreaks. this provides a more accurate match, but sometimes catches following, non-adjacent codeblocks if the intereceding linebreaks aren't appropriately authored. In these cases, we do some extra work in the parser to find and trim any trailing content from the matched syntax.
@rafegoldberg rafegoldberg self-assigned this Apr 28, 2020
@rafegoldberg rafegoldberg merged commit f5f371c into master Apr 28, 2020
@rafegoldberg rafegoldberg deleted the rdmd/next branch April 28, 2020 15:58
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