Skip to content
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

Fully support customize style by classes options #714

Closed
1 of 2 tasks
Jokcy opened this issue Feb 12, 2023 · 3 comments · Fixed by #771
Closed
1 of 2 tasks

Fully support customize style by classes options #714

Jokcy opened this issue Feb 12, 2023 · 3 comments · Fixed by #771

Comments

@Jokcy
Copy link
Contributor

Jokcy commented Feb 12, 2023

Tell us how you think we can improve Sandpack

Currently there is an option classes for people who want to fully customize style for sandpack and remove the default styles generate by stitches. But it seems the feature is not fully supported, by my test there are some problems:

  • some components did not add correct class names: sp-file-expoler/sp-preview etc.
  • code highlight not supported

I want to add fully support to the sp prefixed classes so we can fully customize sandpack appearance using tailwindcss or other class name based style solution.

Packages affected

  • sandpack-client
  • sandpack-react

What is this feature?

Fully support customize sandpack style by classes option.

How would your idea work?

For components, just add correct useClasser class names.

For code highlight, we need to get the full classes map and get correct class name at:

highlightTree(tree, highlightTheme, (from, to, className) => {
    addElement(from, "");
    addElement(to, className);

    // we can do
   addElement(to, `${className} ${classes[className]}`)
  });

The problem is @code-hike/classer did not expose the context and any other hooks to get the whole classes map. Besides the package not being updated in last year so I suppose we can maintain the implementation ourself.

Do you have any examples of how you would like to see us implement it?

No currently.

I will code to PR if you do not have any other concern.

@Jokcy Jokcy added feature:request triage New issues that needs consideration labels Feb 12, 2023
@Jokcy
Copy link
Contributor Author

Jokcy commented Feb 12, 2023

We don't need to get context from classer but from Sandpack context.

@danilowoz
Copy link
Member

Hey, thanks for opening this issue. That is a very interesting point!

Could you open a PR to start a discussion around this problem? We could implement our own version of this classes context helper, which is more lean and extendable. So I'd love to have a starting point where we can iterate on it and learn more about this issue.

To be honest, I think we can make something smarter and take advantage of the classNames helper, which could imply that it adds the prefix and externals classes (our own classes implementation). This would make it easier to maintain and avoid future problems like this, where we forgot to extend the classes on file-explorer and file-preview.

@Jokcy
Copy link
Contributor Author

Jokcy commented Feb 13, 2023

@danilowoz OK, I will start a PR for this. You are right it's better to have a comman pattern to handle all the class name related stuff, I will think about and try to make some design.

@danilowoz danilowoz removed the triage New issues that needs consideration label Feb 27, 2023
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 a pull request may close this issue.

2 participants