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

Sandbox: proper CSS Module import syntax #269

Merged
merged 3 commits into from Feb 10, 2024

Conversation

calebjacob
Copy link
Contributor

Updated the sandbox examples to use proper CSS Module import syntax since it's now supported: #264 (comment)

Anyone who's familiar with CSS Modules will immediately recognize what's going on:

import s from './styles.module.css';

type Props = {
  message?: string;
};

function MyComponent({ message = "Hello!" }: Props) {
  return (
    <div className={s.wrapper}>
      <p>{message}</p>
    </div>
  );
}

export default MyComponent as BWEComponent<Props>;

Copy link

vercel bot commented Feb 9, 2024

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

Name Status Preview Comments Updated (UTC)
bos-web-engine ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2024 0:17am
bos-web-engine-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2024 0:17am

@calebjacob
Copy link
Contributor Author

calebjacob commented Feb 10, 2024

@mpeterdev @andy-haynes It may be worth considering removing support for the following syntax to avoid any confusion:

.wrapper {
  background: red;
}
<div className="wrapper" />
// Since "wrapper" happens to match a class in your styles, it behaves the same as a CSS module class import and automatically gets a randomly generated class name.

This was necessary before we had proper import syntax support. But now that we have proper import support, I feel like the behavior outlined above would be a stumbling block - it wouldn't be expected by any developer who's familiar with CSS Modules.

You could argue that it's convenient to have both options. We'd just need to make sure this behavior is clearly documented (both syntax approaches for CSS).

Any thoughts @thisisjoshford? Happy to explain further if this isn't enough context.

EDIT: This has been resolved here: #271

@andy-haynes andy-haynes merged commit beb71cd into main Feb 10, 2024
5 checks passed
@calebjacob calebjacob deleted the feat/update-css-import-sandbox branch February 12, 2024 15:50
@thisisjoshford
Copy link
Contributor

@mpeterdev @andy-haynes It may be worth considering removing support for the following syntax to avoid any confusion:

.wrapper {
  background: red;
}
<div className="wrapper" />
// Since "wrapper" happens to match a class in your styles, it behaves the same as a CSS module class import and automatically gets a randomly generated class name.

This was necessary before we had proper import syntax support. But now that we have proper import support, I feel like the behavior outlined above would be a stumbling block - it wouldn't be expected by any developer who's familiar with CSS Modules.

You could argue that it's convenient to have both options. We'd just need to make sure this behavior is clearly documented (both syntax approaches for CSS).

Any thoughts @thisisjoshford? Happy to explain further if this isn't enough context.

Although I am a fan of giving devs the option to choose how they want to build, I agree that it could get confusing if we allowed both here. I support going with modules only to keep it simple :)

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.

None yet

3 participants