-
Notifications
You must be signed in to change notification settings - Fork 4
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: ESDS 3.0 release based on Vue 3 and PrimeVue #1461
base: main
Are you sure you want to change the base?
Conversation
@tomleo The basic "hello world" foundation should be ready for a review now. Keep in mind, as per the ticket description, that the scope of this stage is:
I'm particularly interested in any feedback on the names of the makefile commands ( And this PR will remain open for a while (so no need to fully approve it), as it represents the feature branch we'll work against. But worth reviewing what's there so far and any concerns. I've created or will create separate tickets for setting up the initial content in the docs site along with PrimeVue, Jest testing, linting, prettier, simultaneously publishing to NPM via Lerna, etc, as some of those can be done in parallel with developing PrimeVue components. |
I get this error when running mike@redtail es-ds % make dev
npm --prefix es-bs-base link
up to date, audited 3 packages in 445ms
found 0 vulnerabilities
npm --prefix es-ds-components link
up to date, audited 3 packages in 275ms
found 0 vulnerabilities
cd es-ds-docs; npm link @energysage/es-bs-base @energysage/es-ds-components
npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@energysage%2fes-ds-components - Not found
npm ERR! 404
npm ERR! 404 '@energysage/es-ds-components@*' is not in this registry.
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git url.
npm ERR! A complete log of this run can be found in: /Users/mike/.npm/_logs/2024-07-26T16_47_07_229Z-debug-0.log
make: *** [dev] Error 1 |
I got this error too. |
After reviewing the contents of this PR more I now understand it's not meant to be run until the |
Yeah I'm not entirely sure why that error happens - my first assumption was that it was because I hadn't given you an install command to run - I've now updated If running It could be because |
@mpleroux @hroth1994 I just published @energysage/es-ds-components to NPM and properly added it to the package.json of |
|
Yeah I'm seeing those too; the newer version of SASS we're using is just saying some stuff is deprecated and should be cleaned up at some point. A full refactor of es-bs-base is a bit out of scope for this project, though I can make a ticket to have a look if we can clean it up because it makes it tougher to see other errors. |
I have the same experience. I can run it locally and see the messages from |
After reading the Sass page about those Mixed Declarations warnings it doesn't look like an easy fix. Declarations could be reordered within a single file, but conflicts between two files may require restructuring some of Bootstrap's SCSS: βββ> ../../../.nodenv/versions/18.19.1/lib/node_modules/@energysage/es-bs-base/scss/_custom-forms.scss
501β appearance: none;
β ^^^^^^^^^^^^^^^^ declaration
β΅
βββ> ../../../.nodenv/versions/18.19.1/lib/node_modules/@energysage/es-bs-base/scss/mixins/_transition.scss
24 β β @media (prefers-reduced-motion: reduce) {
25 β β transition: none;
26 β β }
β ββββ nested rule I was wondering how Bootstrap themselves were handling it and their solution seems to be... downgrading Sass from 1.7.7.8 to 1.7.7.6? π€¦ |
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.
Nice work on this! I'm glad you were even able to get the Prism source code viewing working.
Regarding the SASS deprecation warning, I don't know why we didn't encounter this before in the POCs. But in any case, I'm not sure it's that complex.. I left a comment on the ticket.
I also wonder whether we should be maintaining the old repos (es-vue-base
and es-design-system
) within this repo. es-vue-base
would be broken if we remove styles from es-bs-base
that are specific to the BootstrapVue implementation. It seems like we could keep the old design system as a branch instead.
es-ds-docs/pages/index.vue
Outdated
here but exists in | ||
<a href="https://v3.primevue.org/introduction/" target="_blank"> | ||
PrimeVue | ||
</a> you are encouraged to use it. |
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.
Sort of a nit: I'm not sure we should encourage using PrimeVue components that are not part of the design system. We should at least warn people that they're gonna be unstyled
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.
Noting that BootstrapVue's implementation for this and other layout components is a bit more complex, and supports other props including offset
and order
as specified in their docs. Do you think we should try to support those props as well? I think the approach you've followed is clearer and has less "magic" than how BootstrapVue does it.
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.
For the final release, probably. I think we and others may be using those props in a few places, but worth a look at how complex it gets.
My main initial goal was to unblock the new design system docs site.
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.
That makes sense yeah
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.
Do you know what this is/why it's necessary? (ditto for analogous files) I tried deleting this file and referencing @energysage/es-bs-base/scss/variables/_blues.scss
from es-ds-docs/pages/atoms/color.vue
as was previously done, and that seemed to work fine
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.
Interesting - when I initially tried it that way, it was coming in as a raw string rather than an object, so I searched around and found this discussion thread that offered some potential solutions.
I just tried switching the import path back to the old file locally and my dev server won't even load the Colors page now - it just hangs.
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.
You restarted the dev server? And your node --version
is 18.19.1
?
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.
I started it from scratch after making the change, yeah. I'm running node 18.12.0
.
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.
Maybe we can troubleshoot that together. Since the .node-version
in this repo is 18.19.1
, Nodenv should automatically put you on that when you're running node or npm from this directory. I'm guessing if you run which node
, you see something other than /Users/yourname/.nodenv/shims/node
I don't know if that's the reason you're encountering this issue, but behavior does change between Node versions, especially the major releases.
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.
Ohhh ok never mind. I'm actually getting the same error when I go specifically to http://localhost:8500/atoms/color. It's strange that it's only a client-side error.
es-ds-docs/nuxt.config.ts
Outdated
|
||
// https://nuxt.com/docs/getting-started/styling#the-css-property | ||
css: [ | ||
'@energysage/es-bs-base/scss/bootstrap.scss', |
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.
Part of what Nuxt Layers allows is to specify this sort of thing in the design system package, rather than in downstream repos. Is there a reason you put the options other than extends
here rather than in es-ds-components
?
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.
Also noting that the previous design system imported bootstrap.import
rather than just bootstrap
. I'm not sure if there's implications to that.
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.
Fair! Having the es-bs-base include come for free with the es-ds-components Nuxt layer would be easier.
Not sure of the difference between bootstrap
and bootstrap.import
- I see a @forward
directive in bootstrap.import
, but not sure that's necessary as I don't think we need to make any variables or mixins available in any SCSS we write "after" the inclusion of bootstrap as there is none, it's passed straight to Nuxt to do a SASS build. In Vue components, I'd expect we still have to do the direct imports of breakpoints
and variables
as we did before.
β Β Linked to Task ESDS-3 Β· es-bs-base@0.1.0 creation |
CED-1764: fix code display and line numbers
π Linked issue
β Type of change
π Description
es-ds-components
package that is a Nuxt layer package and contains the PrimeVue config for downstream Nuxt 3 applications, as well as all related componentses-ds-docs
folder that contains a Nuxt 3 app to power the new design system documentation sitemake dev
π₯Ό Testing
π§ Feedback Requested / Focus Areas
π Checklist