Skip to content

Adjusts component and story to style slotted contents externally#1

Merged
david-poindexter merged 1 commit intodavid-poindexter:dnn-vertical-overflow-menu-storyfrom
valadas:story-overflow-adjust
Mar 20, 2022
Merged

Adjusts component and story to style slotted contents externally#1
david-poindexter merged 1 commit intodavid-poindexter:dnn-vertical-overflow-menu-storyfrom
valadas:story-overflow-adjust

Conversation

@valadas
Copy link
Copy Markdown

@valadas valadas commented Mar 20, 2022

Ok, so the idea is that any item in the component can be a button, a link or anything, for instance another custom element. For that reason we do not want to style those items ourselves inside the component as the external styles do apply to slotted elements. This PR adjusts a couple of things:

  • It brings the consumed CSS variables declarations and fall on the top of the scss file (I have seen this pattern in Ionic quite often and kinda like it because you have a nice overview and it simplifies all the rest of their usage instead of callback declared on each usage).
  • It removes the added styles in the scss file
  • It removes the --text-color customization as really it did not apply anyway, slotted content uses the styles defined externally
  • It adjusts the story for a better example of usage with custom slotted content with their own styles, which might be easier to figure out how to use.

@valadas
Copy link
Copy Markdown
Author

valadas commented Mar 20, 2022

overflow

@valadas
Copy link
Copy Markdown
Author

valadas commented Mar 20, 2022

Oh, it also fixes a height calculation issues, it was counting the gaps but not the margin around the list...

Copy link
Copy Markdown
Owner

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

Agreed 100% - thanks @valadas
🌮🎉

@david-poindexter david-poindexter merged commit 48b0f74 into david-poindexter:dnn-vertical-overflow-menu-story Mar 20, 2022
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.

2 participants