-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add a blog post covering Daniel's work on Spyder 6 and beyond #38
Add a blog post covering Daniel's work on Spyder 6 and beyond #38
Conversation
✅ Deploy Preview for spyder-website-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
da02875
to
d83029c
Compare
d83029c
to
c5cb572
Compare
Hey @conradolandia , what's this strange error we're getting only on GitHub Pages that results in the site build failing? It doesn't appear to my eye to have anything to do with the blog page, and it builds just fine on Netlify...I suspect a new version of NodeJS?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CAM-Gerlach and @dalthviz for your work on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CAM-Gerlach for your work on this! Left a couple of comments and I agree with @ccordoba12 ones.
Also, I was a little bit confused for a moment regarding the blog structure since the images for each section come before the section title so for example while I was reading the Spyder Editor migration to new plugin API
section I saw this:
Which make me think that maybe there was an error with the linked image. To prevent such confusion I would suggest to put first the section name, then the image (if any)
Other than the above this LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reply with revised suggestions to @ccordoba12 and @dalthviz 's reviews.
Was thinking the same about the images; @dalthviz , implemented it in my suggestions as well.
Co-authored-by: Carlos Cordoba <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a suggestion with what seems like a missing change to address a @ccordoba12 suggestion but leaving approved if you think the current state is the correct one. Thanks again @CAM-Gerlach !
Hey @CAM-Gerlach! Yes, it was introduced by a new Node version, they dropped support for CommonJS syntax for imports and instead now we have to use ESM syntax. It is solved by PR #39 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now.
As the title states; created in cooperation with @dalthviz as well as @ccordoba12