feat: immutable records according to designs (wip) #984
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
🦋 Changeset detectedLatest commit: 471aea9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
barthuijgen
force-pushed
the
feat/immutable-records-app
branch
from
339b862 to
0813aac
Compare
| ))} | ||
| </div> | ||
| </main> | ||
| <div className={yearsSidebar}> |
| }, | ||
| ]); | ||
|
|
||
| export const main = style([ | ||
| sprinkles({ | ||
| margin: '$4', |
There was a problem hiding this comment.
hm..als je overal gewoon 'px' gebruikt. zou ik dat bij deze denk ik ook doen.
je zit zover van de react-ui af, dan zou ik het eigenlijk helemaal niet gebruiken.
anders veranderd opeens deze variabele en klopt er niets van je design.
ik zou wel aan vanilla extract vasthouden, want dat gebruiken we nu overal
There was a problem hiding this comment.
isa heeft een npm package '@kadena/fonts' ofzo. daar kun je de kodemono mee gebruiken in css.
het zit ook al in react-ui (font-family: 'mono')
There was a problem hiding this comment.
I checked out trying to use @kadena/fonts together with Isa, but it does not work well with next.js appdir. It is not able to load the font until javascript is executed causing it to pop-in after pageload. Using nextjs's font helper gives the best performance, but it was not compatible with the @kadena/fonts package.
barthuijgen
force-pushed
the
feat/immutable-records-app
branch
from
a6fbd29 to
1cf31df
Compare
| '@typescript-eslint/strict-boolean-expressions': 'off', | ||
| // TODO: eslint in vscode does not understand tsconfig paths and gives errors | ||
| // even though in the build it works fine | ||
| 'import/no-unresolved': 'off', |
There was a problem hiding this comment.
Looks like a good opportunity to have something of a shared app ESLint config, or extend the existing next config.
Those profiles are here: https://github.com/kadena-community/kadena.js/tree/main/packages/tools/eslint-config/profile
This specific import/no-unresolved should not need to be disabled, this works well in the other packages too?
There was a problem hiding this comment.
But I'm already extending next via @kadena-dev/eslint-config/profile/next right?
Also next extends react and react extends lib, so when I use next I automatically opt-in to all the lib rules too
There was a problem hiding this comment.
Also about the import/no-unresolved I think it's specifically the eslint extension in vscode that is missing how to read some configuration, I'm assuming because it's a monorepo and it by default tries to read configs in the root? When I open packages/apps/immutable-records as a new workspace it has no issues.
There was a problem hiding this comment.
But I'm already extending
nextvia@kadena-dev/eslint-config/profile/nextright? Alsonextextendsreactandreactextendslib, so when I usenextI automatically opt-in to all thelibrules too
True. Multiple packages are doing something similar, so perhaps we could share more rules, e.g. some kind of shared next or app ruleset.
Also about the
import/no-unresolvedI think it's specifically the eslint extension in nextjs that is missing how to read some configuration, I'm assuming because it's a monorepo and it by default tries to read configs in the root? When I openpackages/apps/immutable-recordsas a new workspace it has no issues.
Not sure what causes the issue, but import/no-unresolved is an important rule. It does not only resolve your local imports, but also external ones. I think it might catch issues that would not fail the build, only to surface at runtime.
There was a problem hiding this comment.
By the way, it would be fine to create a new ticket to look into all of this later. But either should be fixed here or handled properly.
There was a problem hiding this comment.
Found the solution, you have to configure "eslint.workingDirectories": ["./packages/apps/immutable-records"] in ./vscode/settings.json
Where I just have this app now, but ideally any directory that has a .eslintrc.* is added, really don't know why they don't just resolve these configs automatically but okay, this does work.
| @@ -3,6 +3,7 @@ | |||
| "compilerOptions": { | |||
| "rootDir": "./", | |||
| "types": ["node", "@types/gtag.js"], | |||
| "lib": ["ES2022", "DOM"], | |||
There was a problem hiding this comment.
See comment about shared profiles for ESLint.
barthuijgen
force-pushed
the
feat/immutable-records-app
branch
from
fd22765 to
471aea9
Compare
| @@ -39,6 +47,8 @@ | |||
| "@vanilla-extract/next-plugin": "2.3.1", | |||
| "eslint": "^8.45.0", | |||
| "eslint-config-next": "13.4.5", | |||
| "lokijs": "^1.5.12", | |||
There was a problem hiding this comment.
What is this dependency being used for?
There was a problem hiding this comment.
lokijs and pino-pretty were both required by @walletconnect/modal and @walletconnect/sign-client, it gives build errors when I did not add these, seems like a bug on their side. It's dev-dependencies anyway, I don't think its a big deal right now.
| @@ -39,6 +47,8 @@ | |||
| "@vanilla-extract/next-plugin": "2.3.1", | |||
| "eslint": "^8.45.0", | |||
| "eslint-config-next": "13.4.5", | |||
| "lokijs": "^1.5.12", | |||
| "pino-pretty": "^10.2.0", | |||
There was a problem hiding this comment.
What is this dependency being used for?
There was a problem hiding this comment.
see comment on lokijs, these two have the same reason.
| open, | ||
| onClose, | ||
| }) => { | ||
| useEventListener('keydown', (e) => { |
There was a problem hiding this comment.
Do we really need another dependency to use addEventListener?
There was a problem hiding this comment.
usehooks-ts provides many useful utilities for react apps at a 4kb gziped size. I don't think there should be much reason to have to justify a package of that size if it makes us have to write and maintain less code.
This pull request adds work in progress on immutable records app. I want to make this PR now to avoid it becoming even larger.
The app was already set up as a nextjs project, this PR adds