-
-
Notifications
You must be signed in to change notification settings - Fork 261
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
feat(layer): first pass at Layer
component
#1932
feat(layer): first pass at Layer
component
#1932
Conversation
src/Layer/Layer.svelte
Outdated
const parentLevel = getContext("LAYER_CONTEXT"); | ||
if (level === undefined) { | ||
level = typeof parentLevel === "number" ? parentLevel + 1 : 0; | ||
} | ||
setContext("LAYER_CONTEXT", level); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This does not account for dynamically changing levels, which would require the use of a store in the context and a reactive statement to update the local level/store on change. Not sure if this is worth supporting, though.
- Personally, I would prefix context keys consistently to preempt collisions, e.g. use something like
'carbon.layer'
/'carbon-components.layer'
in this case. If something like this is done, it should apply everywhere, though. So maybe a separate PR would make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the context reactive? Could something like this work?
$: parentLevel = getContext("LAYER_CONTEXT");
$: {if (level === undefined) {
level = typeof parentLevel === "number" ? parentLevel + 1 : 0;
}
setContext("LAYER_CONTEXT", level);
}
Will take advice on the approach to the context name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work, contexts are fixed, they are set during child component construction.
That is why you would need a store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I'll wait to see what others say about whether or not to use stores and how to approach naming.
COMPONENT_INDEX.md
Outdated
| sizes | No | <code>let</code> | Yes | <code>Record<BreakpointSize, boolean></code> | <code>{ | | ||
|
||
sm: false, | ||
md: false, | ||
lg: false, | ||
xlg: false, | ||
max: false, | ||
|
||
}</code> | Carbon grid sizes as an object | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was confused by all the changes here, looks like code sections are not generated correctly. Pretty sure stuff like this breaks the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a platform issue, \r\n
vs just \n
(i.e. Windows vs other systems).
In the JSON file there are now a bunch of \r
, so the \n
of an \r\n
was probably stripped.
If this is correct, the issue might be in sveld.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme know if there is anything I can do on my end to help resolve. I'm on a Win11 system using VSC.
As mentioned in a previous thread, I get heaps of changes in git when I do a rebuild of the docs. Could it be something to do with my configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have the Git setting core.autocrlf
on true
, changing it to false
could help (a new checkout may also be necessary to update the files). This flag causes files to be checked out with \r\n
rather than just \n
, in most cases it should not be necessary, though.
Of course ideally the docs would be generated correctly either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might have resolved it. All the files are still marked "modified" for some reason, but there's no visible changes in the files. I have whitespace diff visible, too.
Not sure what's going on now. Hopefully, the issue just goes away with the next branch I create.
Layer
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thank you!
I've had a crack at implementing the Layer component.
I wasn't sure how to approach porting the
useLayer
function from React.Also, how should we set the
Context
value?