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

Default value for classNamePrefix #4362

Closed
AmirTugi opened this issue Jan 9, 2021 · 7 comments
Closed

Default value for classNamePrefix #4362

AmirTugi opened this issue Jan 9, 2021 · 7 comments
Labels
awaiting-author-response Issues or PRs waiting for more information from the author

Comments

@AmirTugi
Copy link

AmirTugi commented Jan 9, 2021

I couldn't find any information on why would classNamePrefix has no default value.

When the user does not define classNamePrefix - the component-specific classes are not used (e.g - *__multi-value_remove).
Here's a sandbox of it.

This issue related somehow to #4123.

I think that those classes are great to have by default, and this means the user will need 0 config in order to get a specific selector for the internal components.

My use case is this PR for the react-select-event that allows us to find specific components not basing on the type of the component (svg. What if react-select will use a different icon in the future?).

In order to resolve this, all we need is just add here the default value for the classNamePrefix (I suggest react-select).
I can create the PR, and I just ant to know if there's any unseen backward compatibility effect on the user.

I created a PR for react-select-event that uses the internal class names, but it's awaiting this issue to resolve.

@ebonow
Copy link
Collaborator

ebonow commented Jan 9, 2021

Greetings @AmirTugi ,

There are users who prefer to keep their styling within JS/JSX files as opposed to using stylesheets, so in the interest of keeping the DOM cleaner for those individuals, it's likely that classNamePrefix has no default for that reason.

The classNames you are referring to are generated by Emotion and are given hints to correspond to the associated component, but they are not 1:1 with classNamePrefix. In your own examples, you will see that adding a classNamePrefix does not prevent these other classNames from being applied.

So in what way are they different? Let's examine the difference between your examples in the codesandbox. Let's start by looking at the difference in classNames in the Control components.
Screen Shot 2021-01-09 at 11 29 39 AM

Both have a className of css-yk16xz-control. Now let's look when a user clicks on the control to focus it and open the menu for each.

Screen Shot 2021-01-09 at 11 26 25 AM

Screen Shot 2021-01-09 at 11 26 58 AM

Here you'll see that if a classNamePrefix is specified, two new classNames are added. Additionally, you'll notice that the generated className has changed.

Emotion has done the lifting of determining the state changes and generating a new className with the modified styles integrated as a single className. This is by design to mitigate the unintended consequences of cascading styles by instead creating a unique identifier to encapsulate all the changes that should be applied.

So in this way, the classNames are intentionally meant to be obfuscated when a classNamePrefix is not identified, because the underlying logic to display a components changing UI state would no longer correspond to the initial className.

I hope this makes sense and also why providing a default classNamePrefix value would be a disservice to those who choose not to style their components using classNames at the expense of those who choose to use it, having to add a single string prop.

@ebonow
Copy link
Collaborator

ebonow commented Jan 9, 2021

@AmirTugi regarding your question about targeting the indicator icons, these do not necessarily follow the same naming conventions, but I have compiled is a fairly thorough list of options you have when using classNamePrefix in this thread:
#3671 (comment)

These would apply to the styling for the indicators in case you were worried about switching icons.

// applied to all indicators
[prefix]__indicator 

// indicator container
[prefix]__indicators-container

// specific indicator components
[prefix]__clear-indicator
[prefix]__dropdown-indicator
[prefix]__loading-indicator
[prefix]__indicator-separator

In regards to your use-case, I do believe that the selector targeting is a somewhat brittle implementation, so I do agree that this should provide some other means of selecting the clear indicator other than this existing querySelector

  // The "clear" button is the first svg element that is hidden to screen readers
  const clearButton = container.querySelector('svg[aria-hidden="true"]')!;

I'll think a bit more on how to achieve your desired behavior with custom components without requiring a PR.

@AmirTugi
Copy link
Author

I think that keeping the DOM clear for users who do not use styling by classes should be either all or nothing.
I find it very odd that some classes are are applied, and others aren't.
I would like more clarification on the reason, if you may.

Regarding the test selectors -
I think that trying to build a selector using unique indicator like the "first svg that is hidden to screen readers" is very version specific and bound to current implementation.
Next version can change that and use a different default icon that uses an icon font, and without breaking the semantic versioning (why would you consider changed default icon as breaking change?) you break a loosely-coupled library.

That's why I wanted to go by a firmer, more stable indicator.
In my company, we add test-specific prop that helps us catch DOM elements without relying even on classes (as it's CSS related).
data-test-subject.
Perhaps you'd consider adding these to specific elements (I don't believe "keeping the DOM clear" to be a good opposer here).

WDYT?

@ebonow
Copy link
Collaborator

ebonow commented Jan 13, 2021

@AmirTugi No worries. Let me explain a bit further.

The styling of react-select is managed by Emotion.

You can read more about its documentation in the provided link, but the gist of it is to provide a CSS-in-JS solution. Why?
Here is an article detailing some of the use-cases for CSS-in-JS, but essentially it helps avoid global className conflicts while also giving users an ability to encapsulate component styling in a single file. It also overcomes the shortcomings of inline styles by supporting relational and pseudo css-selectors.

There are several other prominent CSS-in-JS libraries (Styled Components, Glam, etc...), but Emotion was chosen for react-select with className and classNamePrefix as an opt-in for users that still choose to use stylesheets, but this is not the use-case for a lot of users. Without specifying the className props, the classNames that do show up are uniquely generated classNames to ensure every specified style corresponds to exactly one className without any nesting. The codesandbox in the screenshots I provided shows that the Control is given the className css-yk16xz-Control but is then changed to css-1pahdxg-Control when the Control is active and the menu is open. It also goes to say that only DOM elements that have styling specified are given classNames.

This represents an issue for react-select-event. It seems that the library has to make assumptions because it appears to only have access to DOM selectors, but the indicators do not have any identifiable attributes outside of their SVG paths

What goes unresolved by this is that a custom component can change any aspect of the expected DOM. Here is a real example that a user requested support for. codesandbox demo

In that respect, I don't think that targeting by SVG path is any less fragile than assuming the clearable button in the first indicator.

@ebonow
Copy link
Collaborator

ebonow commented Jan 13, 2021

@AmirTugi I hope the above resolves the questions you have about what and why. In regards to resolving your use-case for react-select-event, there are a few approaches:

  1. Use the SVG path attributes knowing that the SVG might change or select the first aria-hidden indicator knowing that the DOM hierarchy might change.

  2. Find a way to pass a ref to/from the Select to trigger events.
    I was able to replicate RemoveFirst value functionality with a component ref.
    Demo: codesandbox

  3. If refs are not an option (which may be the case when trying to apply to automated testing), then the best solution in my opinion is to overwrite all of the existing components by adding a special data attribute to the innerProps of each component. I'll see if I can work on an implementation for this, but it would go something like this...

import Select, { components } from 'react-select';
let rseComponents = {};
for (const name in components) {
  rseComponents[name] = (props) => {
    const Component = componentProps => components[name](componentProps);
    const innerProps = { ...props.innerProps, "data-select-component": name };
    const newProps = { ...props, innerProps };
    return <Component {...newProps} />;
  };
}

const rseSelect = (props) => {
  return <Select components={{ rseComponents }} {...props} />;
};

Not quite working yet, but should be a good start on the approach as you could then target by data attributes. I will say that I am not completely familiar with react-select-event so unsure how any of this helps, but hoping this is a viable solution. I should also add that approach number 3 relies on a PR that was approved but ran into some merge conflicts last night which would enable you to apply innerProps to all components.

@ebonow
Copy link
Collaborator

ebonow commented Jan 19, 2021

Greetings @AmirTugi ,

After some thought I have decided that it is might be more consistent to include generated classNames for the indicators. This would produce something to the effect of:

<div aria-hidden="true" class="css-tlfecz-indicatorContainer">
  <svg height="20" width="20" viewBox="0 0 20 20" aria-hidden="true" focusable="false" class="css-6q0nyr-Svg-dropdownIndicator">
    <path d="M4.516 7.548c0.436-0.446 1.043-0.481 1.576 0l3.908 3.747 3.908-3.747c0.533-0.481 1.141-0.446 1.574 0 0.436 0.445 0.408 1.197 0 1.615-0.406 0.418-4.695 4.502-4.695 4.502-0.217 0.223-0.502 0.335-0.787 0.335s-0.57-0.112-0.789-0.335c0 0-4.287-4.084-4.695-4.502s-0.436-1.17 0-1.615z"></path>
  </svg>
</div>

With a className of css-6q0nyr-Svg-dropdownIndicator, this would allow you to target the DOM:

const dropdownIndicator = selectEl.querySelector('class*=dropdownIndicator')

We can continue this discussion in the react-select-events thread, and will see if we can have these changes made as a part of #4342

As such though, the issue raised here is a request for creating a default for classNamePrefix. This is overreaching and not something that should be enforced on all users of react-select simply to provide a DOM hook. Instead, adding a label would allow some level of DOM inspection using a wildcard search by modifying how the uniquely created classNames appear. Please let me know if this works for you and we can have it considered as part of our upcoming 3.3 release.

@ebonow ebonow added the awaiting-author-response Issues or PRs waiting for more information from the author label Jan 20, 2021
@ebonow
Copy link
Collaborator

ebonow commented Feb 5, 2021

We will be closing this issue as this request would introduce an opinionated design decision. I understand that the intention is to develop a reliable way to target DOM nodes based on specific query selectors, but there are ongoing discussions about how to fully disentangle react-select from Emotion which would result in even these existing className hints from all classNames potentially being stripped out.

I think a more solid approach to this would be to expose a a means to apply data attributes to the DOM nodes ie: querySelector('[data-rse-role]=ClearIndicator'). This should be possible by extending all of the innerProps to include this new attribute and will be dependent on #4342 which is planned for released in version 4.1: #4405

I'll continue to keep an eye on this and hope to help out any way we can in providing better testing tools for the community.

@ebonow ebonow closed this as completed Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author-response Issues or PRs waiting for more information from the author
Projects
None yet
Development

No branches or pull requests

2 participants