Skip to content

Conversation

@mcturco
Copy link
Member

@mcturco mcturco commented Jan 3, 2022

Removes extra variable in Container component where classes are rendering twice.

Related issue(s)
Resolves #521

@netlify
Copy link

netlify bot commented Jan 3, 2022

✔️ Deploy Preview for asyncapi-website ready!
Built without sensitive environment variables

🔨 Explore the source changes: 6cccd0b

🔍 Inspect the deploy log: https://app.netlify.com/sites/asyncapi-website/deploys/61d32074cc22870007d2f9c4

😎 Browse the preview: https://deploy-preview-525--asyncapi-website.netlify.app

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

You're fast 😅 LGTM but I see one problem. We should add commonClassNames to the end of the normalClassNames, because at the end we have mx-auto class and if someone will want to override margins (override that class) will have problem, so code should look like:

  const wideClassNames = `max-w-screen-xl`
  const regularClassNames = `max-w-4xl`
  const normalClassNames = `${wide ? wideClassNames : regularClassNames} mx-auto ${commonClassNames}`

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Jan 3, 2022

@mcturco Ok I found that:

Edit to expand: the order the classes appear in the html element has no effect, the only thing that matters is the specificity of the selector and the order in which the selectors appear in the style sheet.

So it's not important in which order we have classes but how tailwind applies them to the stylesheet. Tbh I don't about that, I always thought that from priority is fro left to right 😅 Anyway I will accept it.

@mcturco
Copy link
Member Author

mcturco commented Jan 3, 2022

So it's not important in which order we have classes but how tailwind applies them to the stylesheet.

@magicmatatjahu Yeah I know with traditional CSS that you can add class names in whatever order (kind of like addition) and they would be applied based on cascading order in the stylesheet, which I guess with tailwind if we are loading the default theme mx-auto does appear before the supplemental ml and mr classes. And I guess if we customize that spacing in the tailwind.config.js it would replace the instance there? Curious about that one.

@mcturco
Copy link
Member Author

mcturco commented Jan 3, 2022

/rtm

@magicmatatjahu
Copy link
Member

If something is changed in the tailwind.config.js then yes, but I don't know if it will be overridden by other classes before or after. I don't know when mx-auto is added to the css file.

@mcturco
Copy link
Member Author

mcturco commented Jan 3, 2022

@magicmatatjahu perhaps something we can test against!

Also, sorry for not rebasing with master. I asked @derberg what I should do about it and brought to his attention that I actually cannot see the Update branch button. This is what I see:
Screen Shot 2022-01-03 at 11 34 23 AM

Must be a problem with github config, but are you able to help me merge this one as well? Thanks!

@derberg
Copy link
Member

derberg commented Jan 3, 2022

@mcturco this one is not blocked, as @magicmatatjahu also updated it with latest master so we don't know if you could see something or not. So you can add comment to ready merge

@magicmatatjahu
Copy link
Member

As @derberg wrote :)

@mcturco
Copy link
Member Author

mcturco commented Jan 3, 2022

Sorry @derberg! Haha we wrote at the same time 😆

@mcturco
Copy link
Member Author

mcturco commented Jan 3, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 102626d into asyncapi:master Jan 3, 2022
@mcturco mcturco deleted the container-component-bug branch January 3, 2022 16:41
@derberg
Copy link
Member

derberg commented Jan 13, 2022

@all-contributors please add @mcturco for code, design , ideas, review

@allcontributors
Copy link
Contributor

@derberg

I've put up a pull request to add @mcturco! 🎉

atharva038 pushed a commit to atharva038/asyncapi that referenced this pull request Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Container component class names rendering twice

4 participants