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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some components don't render the passed id prop #518

Closed
namjul opened this issue Dec 17, 2019 · 11 comments 路 Fixed by #520
Closed

Some components don't render the passed id prop #518

namjul opened this issue Dec 17, 2019 · 11 comments 路 Fixed by #520
Labels

Comments

@namjul
Copy link
Contributor

namjul commented Dec 17, 2019

馃悰 Bug report

Current behavior

I used the codesandbox link from https://github.com/reakit/reakit/blob/master/README.md and tested a situation i discovered when using the id attribute in combination with components that use the Hidden component.

The moment i declare an id, it failes to render.

Steps to reproduce the bug

Provide a repo or sandbox with the bug and describe the steps to reproduce it.

  1. Open sandbox: https://codesandbox.io/s/crazy-curran-2x3wc (you can use it as a base)
  2. remove/add the id attribute for Hidden or any other component depending on it.
  3. See console logs

Expected behavior

Should render without failure.

Possible solutions

I currently do not have an propper understanding of the new Id component and how it works to so
i cannot give a possible solution.

Environment

Please, run the command below inside your project directory.

$ npx envinfo --system --binaries --browsers --npmPackages "{react*,reakit*}"

  System:
    OS: Linux 4.18 Ubuntu 18.10 (Cosmic Cuttlefish)
    CPU: (8) x64 Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
    Memory: 796.86 MB / 15.32 GB
    Container: Yes
    Shell: 4.4.19 - /bin/bash
  Binaries:
    Node: 12.10.0 - ~/.asdf/installs/nodejs/12.10.0/bin/node
    Yarn: 1.19.0 - ~/.asdf/installs/nodejs/12.10.0/.npm/bin/yarn
    npm: 6.10.3 - ~/.asdf/installs/nodejs/12.10.0/bin/npm
  Browsers:
    Chrome: 78.0.3904.97
    Firefox: 68.0
  npmPackages:
    react: ^16.8.6 => 16.9.0 
    react-benchmark: ^2.1.0 => 2.1.1 
    react-docgen: ^4.1.1 => 4.1.1 
    react-dom: ^16.8.6 => 16.9.0 
    react-is: ^16.8.1 => 16.9.0 
    react-known-props: ^2.4.2 => 2.4.4 
    react-test-renderer: ^16.11.0 => 16.11.0 
    react-window: ^1.8.1 => 1.8.5 
    reakit: https://pkg.csb.dev/reakit/reakit/commit/35fb93af/reakit => 1.0.0-beta.13 
    reakit-utils: 0.7.2 => 0.7.2 

@open-collective-bot
Copy link

Hey @namjul 馃憢,

Thank you for opening an issue. We'll get back to you as soon as we can.
Please, consider supporting us on Open Collective. We give a special attention to issues opened by backers.

If you use Reakit at work, you can also ask your company to sponsor us 鉂わ笍.

@diegohaz
Copy link
Member

Hi @namjul! I can't reproduce it. If I set id="test" on Dialog, it properly renders the component with the correct id.
image

@namjul
Copy link
Contributor Author

namjul commented Dec 17, 2019

I accidentaly did not provided the correct codesandbox link.
https://codesandbox.io/s/crazy-curran-2x3wc

@diegohaz
Copy link
Member

diegohaz commented Dec 17, 2019

Thanks for the reproduction! The problem there is that you're setting an invalid id (123). IDs can't start with numbers.

The code is throwing on this line:
https://github.com/reakit/reakit/blob/d5e1e8f070c435acdbab92a95a938be23e5803ae/packages/reakit/src/Hidden/__utils/useWarningIfMultiple.ts#L12

This will throw on any website:
image

We could avoid throwing that by wrapping that part of the code within try/catch.

@namjul
Copy link
Contributor Author

namjul commented Dec 17, 2019

Ah true :)

To my situation. I have written a custom singel test based on https://github.com/diegohaz/singel.
When i am using the lastest version provided by codesandbox which i got from here #514 some fail. As you can see here https://codesandbox.io/s/dry-thunder-q52v0 the id attribute does land in the dom in all components. In this example it only works for Menu.

@diegohaz
Copy link
Member

As you can see here https://codesandbox.io/s/dry-thunder-q52v0 the id attribute does land in the dom in all components. In this example it only works for Menu.

Not sure if I got it. Which example you're referring to?

@namjul
Copy link
Contributor Author

namjul commented Dec 18, 2019

If you open the devtools and check the dom elements of each rendered component. Each one should render the id attribute sdf. https://q52v0.csb.app/

@diegohaz
Copy link
Member

I see! So, what鈥檚 the issue?

@namjul
Copy link
Contributor Author

namjul commented Dec 18, 2019

Shouldn't it output the the id i give it? Maybe that is not an important requirement for reakit, so just that you know it does not output the given id.

@diegohaz
Copy link
Member

Oh, now I get what you're saying! Tab, TabPanel and DialogBackdrop aren't rendering the id you provided.

@diegohaz diegohaz added the bug label Dec 18, 2019
@diegohaz diegohaz changed the title Hidden with manual id attribute. Some components don't render the passed id prop Dec 18, 2019
@namjul
Copy link
Contributor Author

namjul commented Dec 18, 2019

The #520 works know for us. Thank you!

diegohaz added a commit that referenced this issue Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants