Skip to content

[Popper + dependents] Add wrapper data attribute - #397

Closed
benoitgrelard wants to merge 3 commits into
mainfrom
add-popper-wrapper-part-data-attr
Closed

[Popper + dependents] Add wrapper data attribute#397
benoitgrelard wants to merge 3 commits into
mainfrom
add-popper-wrapper-part-data-attr

Conversation

@benoitgrelard

@benoitgrelard benoitgrelard commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

Closes #368

This PR adds a data-radix-* attribute to the non-exposed part of Popper (the positioning wrapper).
This is in response to what we have discussed over on this issue: Portalled tooltips appear behind modals/overlays that have z-index #368

I have passed the right name to each dependent component too so you end up with data-radix-tooltip-content-wrapper for example. This way users can precisely target only certain components.

A few questions here:

  • is this enough? What if they only need to set some rules to only "some" tooltips, or "some" menus, etc
  • we could provide a wrapperClassname escape hatch for that I suppose, or even do just that and not bother with the data attribute

Also, doing all this with the part name passed as a prop and the {...getPartDataAttrObj(partName) } internally made me think that perhaps this is a better solution to this issue: [react-*]: Remove extended internal data attributes #347

Thoughts?

@jjenzz

jjenzz commented Jan 14, 2021

Copy link
Copy Markdown
Contributor

I believe @chaance proposed similar for #347 but in that case:

image

🙈 Happy to go with it if you're both considering it though. We can have a selector prop on all our components that defaults to namespaced(partName).

That would allow consumers to rename it if they wanted to which is actually a nice feature and makes me feel more comfortable because we're not just exposing it to fix something internal then, we're giving consumers something useful.

<Accordion selector="my-org-accordion" />
// Outputs
<div data-my-org-accordion />

I would vote for renaming wrapperPartName to wrapperSelector too.

@benoitgrelard

Copy link
Copy Markdown
Contributor Author

Oh that's an interesting idea!

@benoitgrelard

Copy link
Copy Markdown
Contributor Author

Updated as per @jjenzz's feedback and as per upcoming #347 changes.

@benoitgrelard benoitgrelard added the Type: Enhancement Small enhancement to existing primitive/feature label Jan 18, 2021
@benoitgrelard

Copy link
Copy Markdown
Contributor Author

We should wait for #347 to be done, to update this one accordingly as the extra prop might not be warranted as @jjenzz puts it:

jenna 23:43
I noticed we probably don’t need an extra wrapperSelector prop…
23:43
internally you can just do {...getSelectorObj(selector + '-wrapper')} on your wrapper element
jenna 11:08
this should work no? {...(selector ? getSelectorObj(selector + '-wrapper') : undefined}

@jjenzz let me know if you'd rather do this straight in your PR otherwise.

@chaance

chaance commented Jan 18, 2021

Copy link
Copy Markdown
Member

@benoitgrelard One of the benefits of the prop would be for consumers to override our internal selectors if they wanted. This could be very handy if they decide to wrap our components with a different set of naming conventions, and if they'd like to completely close off certain parts in their design systems. We couldn't do that without the selector prop, could we?

@benoitgrelard

Copy link
Copy Markdown
Contributor Author

@jjenzz's point here was that this one is an escape hatch for a part that they normally wouldn't need access to. So it would be fine to derive it from the main part. But yeah that does mean you can't rename it fully like the others.

What do you think Jenna?

@benoitgrelard

Copy link
Copy Markdown
Contributor Author

Closing this one now as #347 has addressed it already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Enhancement Small enhancement to existing primitive/feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Portalled tooltips appear behind modals/overlays that have z-index

3 participants