Skip to content

Update README.md #877

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.

Sign up for GitHub

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

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

Update README.md #877

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2001319
Update README.md
sky-jack May 4, 2023
8d7b6c6
Merge branch 'main' into sky-jack-patch-1
sky-jack Jun 22, 2023
6996ffa
Update README.md
sky-jack Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion toolkits/global/packages/global-layout-with-sidebar/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.


In the following example, the `gap` between the columns is `2em`, the 1-column layout is triggered when the main content goes under `66.666%`, and the sidebar is `300px` wide in the 2-column layout.

Expand Down