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

docs: improved blog section in media #3606

Closed
wants to merge 2 commits into from

Conversation

the-r3aper7
Copy link
Contributor

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests

Description

Previously

Before

Now

After

Use cases and why

    1. One use case
    1. Another use case

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@stackblitz
Copy link

stackblitz bot commented Mar 31, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@the-r3aper7
Copy link
Contributor Author

Is there any way to optimize this?

@zanettin zanettin added the COMP: docs Improvements or additions to documentation label Mar 31, 2023
@zanettin
Copy link
Contributor

Thank you for the adjustments 🙏 imo this looks great ❤️
just my thoughts regarding the implementation: what do you think about adding the values right away to the the blog list instead of 'crawling' them on each request?
so adding the imgSrc as well here: https://github.com/BuilderIO/qwik/blob/main/packages/docs/src/routes/(ecosystem)/media/index.tsx#L156

@mhevery
Copy link
Contributor

mhevery commented Jun 30, 2023

@the-r3aper7 This is a great improvement, but it can't be merged int the current state because node-html-parser is a node package, and the site runs at the edge (Cloudflare) where you can't have runtime dependencies on node packages (This works fine in local development because you are running in node, but will fail in Cloudflare.)

As @zanettin Could this be a build step which fetches these values so that we don't have to do it at runtime?

@the-r3aper7
Copy link
Contributor Author

@the-r3aper7 This is a great improvement, but it can't be merged int the current state because node-html-parser is a node package, and the site runs at the edge (Cloudflare) where you can't have runtime dependencies on node packages (This works fine in local development because you are running in node, but will fail in Cloudflare.)

As @zanettin Could this be a build step which fetches these values so that we don't have to do it at runtime?

yeah I was busy for days i will make it as static input

@the-r3aper7 the-r3aper7 deleted the improve-media branch June 30, 2023 17:32
@the-r3aper7 the-r3aper7 mentioned this pull request Jun 30, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants