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

Button to add a drive to the storage proposal #1977

Open
wants to merge 1 commit into
base: storage-config-ui
Choose a base branch
from

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Feb 5, 2025

Adds the following button to the new storage UI.

addDrive

If there are no extra devices to add, the button is not displayed.

Missing

Unit tests are still missing

@ancorgs ancorgs force-pushed the storage-ui-add-disk branch from 2ce80c3 to 6cd9c15 Compare February 5, 2025 15:25
@ancorgs
Copy link
Contributor Author

ancorgs commented Feb 5, 2025

@dgdavid The tests are failing due to PatternFly type definitions. The property label only accepts a string for SelectGroup and for DropDownGroup. Although it accepts a ReactNode for MenuGroup.

In fact it works just fine, so the type restriction looks kind of arbitrary to me.

Is there a way to fix it without having to migrate the whole component to use a menu instead of a dropdown?

*
* @param device - Device to represent
*/
export default function MenuDeviceDescription({ device }): React.ReactNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, define a MenuDeviceDescriptionProps type.

@dgdavid
Copy link
Contributor

dgdavid commented Feb 6, 2025

@dgdavid The tests are failing due to PatternFly type definitions. The property label only accepts a string for SelectGroup and for DropDownGroup. Although it accepts a ReactNode for MenuGroup.

In fact it works just fine, so the type restriction looks kind of arbitrary to me.

Is there a way to fix it without having to migrate the whole component to use a menu instead of a dropdown?

Looking at their code base and git history, look to me as an out-dated prop.
It's is true that internally SelectGroup is using MenuGroup. But hard to say whether it's label type is intentional or not.

What I can see is that MenuGroup#label was an string until two years ago, patternfly/patternfly-react#8219 and the component itself was added 5 years ago patternfly/patternfly-react@e867f09 While the SelectGroup#label types dates from 6 years ago.

So, maybe a bug report for PatternFly :)

Meanwhile, I'd migrate it to Menu if it does not hurt or, if you prefer, add a /** @ts-expect-error: reason text */ comment.

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Would be nice if you add these missing tests 😈

@ancorgs ancorgs force-pushed the storage-ui-add-disk branch from de9092f to e581abd Compare February 6, 2025 09:59
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.

3 participants