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

feat(layer): first pass at Layer component #1932

Merged

Conversation

SimpleProgrammingAU
Copy link
Contributor

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?

src/Layer/Layer.svelte Outdated Show resolved Hide resolved
src/Layer/Layer.svelte Outdated Show resolved Hide resolved
src/Layer/Layer.svelte Show resolved Hide resolved
Comment on lines 28 to 32
const parentLevel = getContext("LAYER_CONTEXT");
if (level === undefined) {
level = typeof parentLevel === "number" ? parentLevel + 1 : 0;
}
setContext("LAYER_CONTEXT", level);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 357 to 365
| 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 |
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@metonym metonym changed the title V11-Layer first pass for discussion feat(layer): first pass at Layer component Mar 23, 2024
Copy link
Collaborator

@metonym metonym left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@metonym metonym merged commit 180f33b into carbon-design-system:next/v11-styles Mar 23, 2024
1 check passed
metonym pushed a commit that referenced this pull request Mar 23, 2024
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.

None yet

3 participants