jsonresume / jsonresume.org

The mono repo that builds the homepage, utils, ui components, registry and anything else
https://jsonresume.org
68 stars 18 forks source link

Add jsonresume-theme-relaxed #22

Closed ObserverOfTime closed 9 months ago

ObserverOfTime commented 9 months ago

Closes jsonresume/registry-functions#72

Summary by CodeRabbit

vercel[bot] commented 9 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jsonresume-org-homepage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 0:51am
jsonresume-org-registry ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 0:51am
vercel[bot] commented 9 months ago

@ObserverOfTime is attempting to deploy a commit to the JSON Resume Team on Vercel.

A member of the Team first needs to authorize it.

coderabbitai[bot] commented 9 months ago

Walkthrough

The changes involve updating a JavaScript object to include a new theme called 'relaxed' and repositioning an existing theme 'riga'. Additionally, a minor syntax update has been made to the commented-out 'eloquent' theme, changing double quotes to single quotes.

Changes

File Path Change Summary
.../api/formatters/template.js Added 'relaxed' theme, moved 'riga' theme, updated 'eloquent' theme comment syntax

Assessment against linked issues

Objective Addressed Explanation
Add jsonresume-theme-relaxed (#72) The 'relaxed' theme has been added to the THEMES object, which aligns with the issue's title and description.

Poem

In the code where themes align,
A 'relaxed' one joins the line.
'Riga' moves with subtle grace,
Quotes 'round 'eloquent' now embrace. 🐰✨


Tips ### Chat with CodeRabbit Bot (`@coderabbitai`) - If you reply to a *review comment* from CodeRabbit, the bot will automatically respond. - To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment - Note: Review comments are made on code diffs or files, not on the PR overview. - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Note: For conversation with the bot, please use the review comments on code diffs or files. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json`
ObserverOfTime commented 9 months ago

I see the issue. My theme expects the skill level to be one of these but the sample file has 'Senior'. I'll publish a new version that supports it. Would it be possible to standardise skill levels in the future?

levino commented 9 months ago

I would hold off with changes to your code for the time being. I am currently digging into how to test this and it turns out it is not so easy. We need to be able to test in order to see what is the problem with your code. If not you might change things for no reason. Did you locally test your changes to this repo? If so: how?

I am also quite new to this project, was made collaborator just a few days ago.

ObserverOfTime commented 9 months ago

I tested it by running pnpm dev --filter=registry and visiting http://localhost:3002/thomasdavis?theme=relaxed. This results in Error: Unexpected skill level: Senior which means the issue is on my side. I was making some other changes anyway so I'll publish a new version and see if that fixes it.

levino commented 9 months ago

I think you rather mean pnpm --filter=registry dev. However the page crashes with a problem with the apage theme for me (it tries to access process.env.LANG) which does not exist.

ObserverOfTime commented 9 months ago

I think you rather mean pnpm --filter=registry dev.

That's not what the readme says.

(it tries to access process.env.LANG) which does not exist.

Just set LANG=C LANG=en.


Anyway, I've fixed it.

levino commented 9 months ago

I think you rather mean pnpm --filter=registry dev.

That's not what the readme says.

I ran the command from apps/registry and there it only works the other way round. My bad, sorry. Please also see #26 for details.

(it tries to access process.env.LANG) which does not exist.

Just set ~LANG=C~ LANG=en.

Even though this might be a workaround, I find it quite problematic to rely on a variable that is only set automatically in certain environments (e.g. specific linux distros) and would rather remove the theme.

Anyway, I've fixed it.

Waiting for #25, then I will update your branch and then we can go ahead.

levino commented 9 months ago

I would assume that the fs.readFile (sync or not sync) is the problem in a serverless environment since the code propably gets bundled and only files that are correctly imported will be bundled with. @thomasdavis said something about problems for handlebars and reading from the file system as well, iirc.

ObserverOfTime commented 9 months ago

I ran the command from apps/registry and there it only works the other way round.

I think the filter parameter is ignored in that case.

Even though this might be a workaround, I find it quite problematic to rely on a variable that is only set automatically in certain environments (e.g. specific linux distros)

It seems to be used by the theme for i18n though.

I cannot reproduce this error locally.

I'll look into it again tomorrow but I haven't encountered it so far.

ObserverOfTime commented 9 months ago

I cannot reproduce it locally but it shouldn't be an issue with fs.readFile since the other usages work. It's most likely an unresolved stylus issue (stylus/stylus#2711).

ObserverOfTime commented 9 months ago

I published a new version that precompiles the stylesheet. Hopefully it'll work now.

thomasdavis commented 9 months ago

It still is throwing a 500 -> https://jsonresume-org-registry-4wi0nemyq-jsonresume.vercel.app/thomasdavis?theme=relaxed

Error: stylus:280:17
   276|       float right
   277| 
   278|   html[dir="rtl"]
   279|     #image > img, .float
   280|       float left
------------------------^
   281| 

failed to locate @import file /var/task/node_modules/.pnpm/stylus@0.62.0/node_modules/stylus/lib/functions/index.styl

    at Evaluator.visitImport (/var/task/node_modules/.pnpm/stylus@0.62.0/node_modules/stylus/lib/visitor/evaluator.js:911:23)

Will have a look too

ObserverOfTime commented 9 months ago

Weird, it shouldn't be using stylus anymore. :thinking: Could you post the full stack trace? Maybe it's still running the old version?

ObserverOfTime commented 9 months ago

Looks like it works now. :tada:

levino commented 9 months ago

It does. @thomasdavis I think you forgot to approve the redeployment of the preview of the registry after @ObserverOfTime pushed their changes with the new version 1.0.6 of their theme. I approved the deployment with the new changes and it is working indeed. Merging.

thomasdavis commented 9 months ago

Ah yep, works for me. Thanks!