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

Update README.md #877

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update README.md #877

wants to merge 3 commits into from

Conversation

sky-jack
Copy link
Contributor

@sky-jack sky-jack commented May 4, 2023

I wanted to propose an extra line to this documentation, as being fairly new to CSS custom properties it took me a bit of time to realise how to best to adjust these values using media queries and though useful for the purpose of demonstration, adding the properties inline seems to make this difficult.

I wanted to propose an extra line to this documentation, as being fairly new to CSS custom properties it took me a bit of time to realise how to best to adjust these values using media queries and though useful for the purpose demonstration, adding the properties inline seems to make this difficult.
@@ -51,7 +51,7 @@ Somewhat unconventional, but the component can take it!

### Custom settings

The `global-layout-with-sidebar` component’s settings are mapped to CSS custom properties, meaning you can alter them inline.
The `global-layout-with-sidebar` component’s settings are mapped to CSS custom properties, meaning you can alter them inline. You can also set custom properties within your stylesheet and modify them using media queries to adjust for different view widths.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it strange that we're encouraging overriding these values with inline styles 🤔.
Would it be better not to mention inline at all, e.g. meaning you can override them in your stylesheets, and then provide example scss code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the use case for being able to override these? and agree and example would be useful

Copy link
Contributor Author

@sky-jack sky-jack May 4, 2023

Choose a reason for hiding this comment

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

I am also not quite sure of the use case for adjusting these custom properties inline and from my limited experience with this would agree with providing another example, but its possible I'm missing something here.

Copy link
Contributor

@sturobson sturobson May 5, 2023

Choose a reason for hiding this comment

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

I think having the ability to change the value for --with-sidebar--basis in. your projects stylesheet to override the default value is fine. You may find, depending on how often and where you're using this layout you might need a couple of variations of it set in the parent component (or the component) class.

<article class="eds-c-summary | eds-c-with-sidebar"></article>

or

<article class="eds-c-summary">
  <div class="eds-l-with-sidebar"></div>
</article>

can be targeted like:

.eds-c-summary {
  // this will target the css custom property for the sidebar, and any child elements (because of the Cascade).
  --with-sidebar--basis: 34rem;
}

You can also change --with-sidebar--gap if needed too.

As the component is an intrinsic layout (where you offer the browser guidance rather than strictly dictate what you want it to do) I don't think adding a media query is in the spirit of what the component is set out to do,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this suggestion, I've made a further change to reflect this.

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.

4 participants