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

feat: display of relative paths in markdown and improvement in file preview in markdown #1008

Merged
merged 4 commits into from
Sep 7, 2020

Conversation

vfried
Copy link
Contributor

@vfried vfried commented Aug 13, 2020

I created a markdown component that fetches images from relative paths and also fixes relative links in markdown files.

I also improved/restyled the preview of files...

We only preview files with "!" in the front... since I think it's nice to also leave the option to not preview a file in case this is not desired.

image

Closes #667
Closes #941

@vfried vfried requested a review from a team as a code owner August 13, 2020 21:26
@vfried vfried force-pushed the 667-paths-in-markdown branch 3 times, most recently from aaedd85 to eed9081 Compare August 18, 2020 12:33
@vfried
Copy link
Contributor Author

vfried commented Aug 18, 2020

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Been testing this, and it is very nice, a major improvement to how it worked before!

I noticed a couple of discrepancies to how images are handled in GitLab, and I think we should decide what to do about them.

  1. In GitLab, relative paths work (e.g., I can do ../images/foo.png that it will be resolved correctly), but this does not work in RenkuLab
  2. In GitLab, images are set to max-width: 100%, so they never overflow, but they are shown full size in RenkuLab
  3. In GitLab, images that are in LFS are rendered in markdown files, but this does not work in RenkuLab
  4. Images are not rendered correctly in issue previews.

To see illustrations of these, compare the following pairs:

Point 1
RenkuLab: something.md and GitLab: something.md

Points 2, 3
RenkuLab: README.md and GitLab: README.md

Point 4
https://virginiatest.dev.renku.ch/projects/cramakri/markdown-images/collaboration/issues

I think we should make relative paths work in RenkuLab (1). For the max-width (2), I think the way it is handled in GitLab is a bit nicer, but I don't think it is a dealbreaker either. If handling LFS files is easy (3), I think it would be nice to have, but if it is difficult, it can be done later. I think we should show the alt-text for an image in an issue preview (4).

Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Code looks good! I have two small things to fix: I think I found a leftover debugging comment, and there is a name I do not understand, but otherwise looks great.

src/model/RenkuModels.test.js Show resolved Hide resolved
src/utils/Markdown.js Outdated Show resolved Hide resolved
@ciyer ciyer self-requested a review September 7, 2020 06:42
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

The fixes for the relative paths handle the most common case, which is great! Two change requests:

  • Rename RenkuMarkdownWithFiles -> RenkuMarkdownWithPathTranslation (I think this is a clearer name)
  • Could you write a couple of tests for the fixRelativePath function?

@vfried vfried force-pushed the 667-paths-in-markdown branch 2 times, most recently from f2e30b4 to 9f995ea Compare September 7, 2020 11:02
@vfried
Copy link
Contributor Author

vfried commented Sep 7, 2020

I addressed the requested changes and fixed a small bug I found with ids for file previews in issues.

@vfried vfried requested a review from ciyer September 7, 2020 11:04
Copy link
Contributor

@ciyer ciyer left a comment

Choose a reason for hiding this comment

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

Looks good! If you squash, don't forget to add a reference to the issue #667 and the PR #1008 in the commit message.

@vfried vfried merged commit b945402 into master Sep 7, 2020
@ciyer ciyer linked an issue Sep 9, 2020 that may be closed by this pull request
ciyer pushed a commit that referenced this pull request Sep 9, 2020
…review in markdown (#1008)(#941)(#667)

* feat: display of relative paths in markdown and improvement in file preview

* fixes and improvements

* fixes in relative paths

* fixes and test

(cherry picked from commit b945402)
@ciyer ciyer deleted the 667-paths-in-markdown branch October 22, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants