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

Update dependencies #85

Merged
merged 18 commits into from
Feb 12, 2024
Merged

Update dependencies #85

merged 18 commits into from
Feb 12, 2024

Conversation

mashehu
Copy link
Collaborator

@mashehu mashehu commented Jan 3, 2024

  • Update to astro 4, which requires changes in the docs (relative links for images stopped working), PR is incoming. Broke also the rehype-inline, so needed an astro element to render svgs inline.
  • Includes also an update to astro-icon 1.0 and rehype-pretty-code, which required some changes.
  • Updated also all other dependencies

@mashehu mashehu requested a review from ewels January 3, 2024 14:12
Copy link

netlify bot commented Jan 3, 2024

Deploy Preview for multiqc failed.

Name Link
🔨 Latest commit 0021f08
🔍 Latest deploy log https://app.netlify.com/sites/multiqc/deploys/65c9f9aa06a21a0008434d1c

mashehu added a commit to mashehu/MultiQC that referenced this pull request Jan 3, 2024
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Looking good! Lots of changes, but great to keep pace with Astro development.. 🚀 🧑‍🚀

sitemap(),
svelte(),
icon({
include: {
// Include only subset of icon bundles
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this chunk of config? To speed up builds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Right, the icon() bit is. But not the include:..?

Copy link
Member

Choose a reason for hiding this comment

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

As in, what's the benefit over this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

astro.config.mjs Outdated Show resolved Hide resolved
astro.config.mjs Outdated Show resolved Hide resolved
astro.config.mjs Show resolved Hide resolved
src/components/Footer.astro Outdated Show resolved Hide resolved
src/components/Hero.astro Show resolved Hide resolved
src/docs/images/MultiQC_logo.png Outdated Show resolved Hide resolved
src/pages/index.astro Outdated Show resolved Hide resolved
Comment on lines 53 to 55
class="logo w-full cursor-pointer"
width="550px"
height="110px"
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
class="logo w-full cursor-pointer"
width="550px"
height="110px"
class="logo h-28 max-w-full cursor-pointer"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Astro-icons needs a width, so I will use w-full instead of the proposed max-w-full

Copy link
Member

Choose a reason for hiding this comment

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

ok, as long as that doesn't mess with the aspect ratio. If they can do without a height then you could instead do this:

                class="logo w-[550px] max-w-full cursor-pointer"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needs also a height, but then your code works and is added in 0418564

@mashehu
Copy link
Collaborator Author

mashehu commented Jan 8, 2024

Both PRs should be good to go now

astro.config.mjs Show resolved Hide resolved
@ewels
Copy link
Member

ewels commented Feb 12, 2024

Netlify was pinned on Node 16, which I think is why the build was failing. Just updated to Node 20

@ewels
Copy link
Member

ewels commented Feb 12, 2024

gah, settings and also in the netlify toml file 😅

@ewels
Copy link
Member

ewels commented Feb 12, 2024

ok, failing but for a different reason now - needs the PR from MultiQC itself. Will merge and see if I can get everything to build. Thanks!

@ewels ewels merged commit 61f4f75 into MultiQC:main Feb 12, 2024
0 of 4 checks passed
ewels added a commit to MultiQC/MultiQC that referenced this pull request Feb 12, 2024
* changes necessary with the update astro version

* changes necessary with the updated astro version

needs to be merged together with MultiQC/website#85

* [automated] Update CHANGELOG.md

* jump around a bit more to make relative links work

* fix svg, needs to be remote 😞

---------

Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>
Co-authored-by: Phil Ewels <phil.ewels@seqera.io>
@mashehu mashehu deleted the update-dependencies branch February 12, 2024 12:21
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.

None yet

2 participants