Skip to content

Conversation

@Nivb1569
Copy link
Contributor

@Nivb1569 Nivb1569 commented Apr 3, 2025

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the bun lint on the code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • For visual changes, I've added screenshots to the PR.
  • I accept that @tmr232 may be pedantic in the code review.

Description

Pull Request Summary

Added onMount import (line 10): This ensures the color scheme is updated only once when the page loads.

Initialized color scheme (line 83): Now retrieves the color scheme when the demo first loads.
I chose not to place this in the run function, as doing so would prevent users from updating the color scheme later via state changes.

Added support for new query parameters in the URL:

fontSize

simplify

flatSwitch

highlight

version

colorList (compressed)

URL version:
Introduced to support future changes to the URL structure without breaking backward compatibility for shared links.
When a new format is introduced, the currentVersion variable should be incremented accordingly.

Moved fontSize parsing (from line 112 to line 74):
This was done to ensure it’s parsed early with the other URL parameters.

URL structure:
fontSize, simplify, flatSwitch, and version are added as-is to the URL to save space, while colorList is compressed.

Fixed language comparison bug (previously line 129 → now line 169):
The condition now compares language values rather than objects.
This solves a bug where clicking “Share” again after loading a shared link would incorrectly reset the language to the default (Go).

Changed the “Share (experimental)” label to simply “Share”.

Updated CHANGELOG.md with all relevant changes.

Closes #84

Copy link
Owner

@tmr232 tmr232 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

It seems to work well, but there are a few changes I want to make.

I think the entire shared data should go in a single query parameter.
That single parameter should be a JSON serialized object with all the rest of the data inside it, including the version number.
That way, when there is an update, we can modify things without the users seeing that or being affected.

Additionally, I want users to still be able to link to a specific language (not code, just a language setting) as it makes it easier to tell people "here, use this with C code" if they don't need to change the selected language.

Please tell me if you have any further questions - it seems I was not clear enough the first time, and I don't want to make you do duplicate work.

@Nivb1569
Copy link
Contributor Author

Nivb1569 commented Apr 4, 2025

Hi Tamir!
Thanks for the feedback.
We appreciate the time you took to review the PR.
We will make the changes you suggested as soon as we can!

@Nivb1569 Nivb1569 requested a review from tmr232 April 10, 2025 20:11
@Nivb1569
Copy link
Contributor Author

Hi Tamir,
I fixed all the changes you requested and refreshed the PR. Let me know if there's anything else you'd like me to update.

Copy link
Owner

@tmr232 tmr232 left a comment

Choose a reason for hiding this comment

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

This is looking much better!

There are still some changes I'd like to see though.
Mostly around error handling and code structure.

Thank you for contributing!

@Nivb1569 Nivb1569 requested a review from tmr232 April 14, 2025 13:21
@Nivb1569
Copy link
Contributor Author

Hi Tamir, how are you?
I’ve implemented all the requested changes.
Please feel free to let me know if anything is missing or need anything else.

Copy link
Owner

@tmr232 tmr232 left a comment

Choose a reason for hiding this comment

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

This is looking much better, great work!

Still a few things to fix though, and you need to update the branch.

Also - feel free to add yourself to the CONTRIBUTORS.md file.

@Nivb1569 Nivb1569 requested a review from tmr232 April 16, 2025 14:19
@Nivb1569
Copy link
Contributor Author

Hi Tamir,
I fixed all the changes you requested and refreshed the PR.
Thank you!

Copy link
Owner

@tmr232 tmr232 left a comment

Choose a reason for hiding this comment

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

So the code looks a lot nicer, and I don't really have comments on that.
But it seems that this PR breaks the color-picker feature.

With this patch, if you use the color-picker to change the colors - they don't change at all.
Please solve this bug.

@Nivb1569
Copy link
Contributor Author

Hi Tamir,
I made some tests and everything seems to be working correctly on my side.
Could you share more details about the situation where the color-picker breaks please?
Maybe you have a specific URL I can check on my side?
Thanks.

@tmr232
Copy link
Owner

tmr232 commented Apr 16, 2025

If you open the color-picker and change a color, it should update live. When I try your PR locally - it does not.
Later, if I close the color-picker, no color is updated.
When I open it again - the colors are reset to their original values.

@Nivb1569
Copy link
Contributor Author

It's really weird everything seems to be working fine on my side.
I recorded a quick screen capture to show that the color picker is working correctly:
https://drive.google.com/file/d/1-iiD7di0863vglizxpUHtQd1UwJYXYNS/view?usp=sharing
Let me know if you have any idea what be causing the issue on your side.
Thanks!

@tmr232
Copy link
Owner

tmr232 commented Apr 17, 2025

Try running bun install --frozen-lockfile to make sure all your deps are up to date, and run it again. Maybe something got out of sync?

@Nivb1569
Copy link
Contributor Author

I tried running bun install --frozen-lockfile, and everything still looks good on my side.
Maybe we can jump on a quick Zoom call whenever you want and try to figure it out together?

Copy link
Owner

@tmr232 tmr232 left a comment

Choose a reason for hiding this comment

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

I have no idea what changed - but it works on my machine now.

@tmr232 tmr232 merged commit 5b574a4 into tmr232:main Apr 17, 2025
7 checks passed
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.

Include settings & color-scheme when sharing in the demo

2 participants