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(eslint-plugin-rules): checking for ouiaId in pf4 components only #470

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

MariaAga
Copy link
Member

This eslint rule failed in foreman core for components with the same name, that were imported from somewhere else, like from patternfly-react or just, for example import Text from '../Text'; .

@MariaAga
Copy link
Member Author

@lfu @jeremylenz Can you take a look?

@MariaAga
Copy link
Member Author

MariaAga commented Jun 6, 2023

Added Tab to the list as it was requested by QE to have ouia-ids in it as well

@lfu
Copy link
Contributor

lfu commented Jun 6, 2023

@MariaAga How do I recreate the eslint rule failure?

Copy link
Contributor

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Tested with the katello PR to add ouia-Ids to tabs. Able to get missing ouia_id warnings with this PR when ouia-Id is missing. Ack based on my testing👍🏼

@MariaAga
Copy link
Member Author

MariaAga commented Jun 7, 2023

@lfu, have a React component named Text, and import it
the eslint rule before this change fails.
That can also cause issues as it might lead to errors it the component passes down all its props like in this example:
Text.js

import React from 'react';
export const Text = props = <div {...props)>Text</div>

index.js

import React from 'react';
import { Text } from './Text'

const Component = <Text />

@lfu
Copy link
Contributor

lfu commented Jun 9, 2023

@MariaAga I can't reproduce the eslint failure.

[vagrant@centos8-katello-devel test]$ pwd
/home/vagrant/foreman/webpack/assets/javascripts/react_app/components/HostDetails/Tabs/test
[vagrant@centos8-katello-devel test]$ ll
total 8
-rw-rw-r--. 1 vagrant vagrant 117 Jun  9 15:12 index.js
-rw-rw-r--. 1 vagrant vagrant  85 Jun  9 15:04 Text.js
[vagrant@centos8-katello-devel test]$ more index.js 
import React from 'react';
import { Text } from './Text';

const Component = <Text name="lucy" />;
return Component;
[vagrant@centos8-katello-devel test]$ more Text.js 
import React from 'react';

export const Text = props => <div {...props}>Text</div>;
[vagrant@centos8-katello-devel test]$ npm run lint

> TheForemanDevDeps@3.8.0 lint /home/vagrant/foreman
> tfm-lint

> eslint finished without any errors!

@MariaAga
Copy link
Member Author

@lfu looks like youre testing in foreman, foreman doesn't use the ouiaId rule yet, I want to add it to Foreman after this pr so when we add it we'll only add ids where needed. I pasted your example in Katello and got ouiaId property is missing

@lfu
Copy link
Contributor

lfu commented Jun 16, 2023

@MariaAga
I did see the warning ouiaId property is missing when testing inside Katello. And the message went away when ouiaId was added.
That was tested without your patch. So the eslint rule seems work well even with components with the same name, that were imported from somewhere else.
Maybe I have missed something.

@MariaAga
Copy link
Member Author

@lfu from the Readme file, I see that we only want to add ouiaid to pf4 ouiaid complied components. So from what I understand we shouldn't add the ouiaid to other components, so the lint rule should not "make" us add the ouiaid

@adamruzicka
Copy link
Contributor

Soo, is this good to go?

@jeremylenz jeremylenz merged commit afe1c7f into theforeman:master Jun 28, 2023
14 checks passed
@github-actions
Copy link

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

5 participants