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

CSV embed using wiki-link transclusion format #1287

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gradedSystem
Copy link
Member

@gradedSystem gradedSystem commented Aug 22, 2024

Changelog

Added

  • CSV Embedding: Implemented support for embedding CSV files using ![[data.csv]] syntax. CSV files are rendered as tables in markdown.

Updated

  • fromMarkdown.ts: Modified to handle CSV files by rendering them as tables.
  • html.ts: Updated to output CSV files as HTML tables when using the ![[data.csv]] syntax.

Reference

Issue #1233

cc @olayway

Copy link

changeset-bot bot commented Aug 22, 2024

⚠️ No Changeset found

Latest commit: 9959bff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Aug 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
portaljs-alan-turing ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 0:34am
portaljs-ckan ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 0:34am
portaljs-git-example ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 0:34am
portaljs-learn ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 0:34am
portaljs-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 0:34am
site-portaljs ⬜️ Ignored (Inspect) Visit Preview Aug 29, 2024 0:34am

Copy link

vercel bot commented Aug 22, 2024

@gradedSystem is attempting to deploy a commit to the Datopian Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@olayway olayway left a comment

Choose a reason for hiding this comment

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

Please add tests.

@gradedSystem
Copy link
Member Author

@olayway added test cases

@olayway olayway changed the title [#1233 extend feature][xs] Adding markdown-transclusion. CSV embed using wiki-link transclusion format Aug 28, 2024
Copy link
Member

@olayway olayway left a comment

Choose a reason for hiding this comment

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

Some changes are needed, especially to html lib and it's test suite.

Also, please update the README, including instructions on any requirements for using this csv embeds. I suggest trying to use it e.g. in our Flowershow template and document any steps you had to take to make it work.

Comment on lines 23 to 27

test("should return [true, <extension>] for a path with supported file extension", () => {
const filePath = "image.csv";
expect(isSupportedFileFormat(filePath)).toStrictEqual([false, "csv"]);
});
Copy link
Member

Choose a reason for hiding this comment

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

This is already covered by the test with the same title above.

Suggested change
test("should return [true, <extension>] for a path with supported file extension", () => {
const filePath = "image.csv";
expect(isSupportedFileFormat(filePath)).toStrictEqual([false, "csv"]);
});

Comment on lines 170 to 172
expect(serialized).toBe(
'<p><a href="data.csv" class="internal new" download="data.csv">data.csv</a></p>'
);
Copy link
Member

Choose a reason for hiding this comment

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

Why is it a regular a tag and not FlatUiTable? Isn't it the aim of this PR?

);
});

test("parses a CSV file embed of unsupported file format", () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test("parses a CSV file embed of unsupported file format", () => {
test("leaves a CSV file embed of unsupported file format as plain text", () => {

htmlExtensions: [html({ permalinks: ["data.csv"] }) as any], // TODO type fix
});
expect(serialized).toBe(
'<p><a href="data.csv" class="internal" download="data.csv">data.csv</a></p>'
Copy link
Member

Choose a reason for hiding this comment

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

Again, why does it include a regular a tag?

htmlExtensions: [html() as any], // TODO type fix
});
expect(serialized).toBe(
'<p><a href="data.csv" class="internal new" download="data.csv">My CSV File</a></p>'
Copy link
Member

Choose a reason for hiding this comment

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

???

If anything, alias should be used as table title. But I think you can just ignore it for now.

@gradedSystem
Copy link
Member Author

Fixed the issues with tests @olayway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants