Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(markdown): fix various markdown issues #370

Merged
merged 4 commits into from
Sep 28, 2017
Merged

fix(markdown): fix various markdown issues #370

merged 4 commits into from
Sep 28, 2017

Conversation

machour
Copy link
Member

@machour machour commented Sep 26, 2017

Fix #363

The problems were :

  • Text inside <th> was HTML escaped: it needed to be displayed using defaultRenderer
  • An <img> inside a <td> caused a crash (View in Text): We created our own Cell component that avoids using <Text>
  • marked() was rendering a simple piece of code as a <pre> if inside a <li>: Setting it to be pedantic helped

I also made images inside cells take 30% of the screen width while enlarging a bit images outside cells.

class CellWithImage extends Cell {
render() {
const { data, width, height, flex, style } = this.props;
let borderWidth;
Copy link
Member

Choose a reason for hiding this comment

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

Do you think, if you set the variables (borderWidth, borderColor) to default values, and thus remove the else branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -266,19 +306,33 @@ export class MarkdownHtmlView extends Component {
styleText.textAlign = cellAlign[attribs.style];
}

const Component =
node.children.filter(
elem => elem.type === 'tag' && elem.name === 'img'
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be possible to move this arrow function in a separate function, if it is used more than once?

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

@andrewda andrewda changed the title Markdown fixes fix(markdown): fix various markdown issues Sep 27, 2017
Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Beautiful, thank you so so much @machour. You single-handedly made comments look beautiful in the app 🙌

@housseindjirdeh housseindjirdeh merged commit 21b6ea6 into gitpoint:master Sep 28, 2017
@machour machour deleted the markdown-fixes branch September 28, 2017 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants