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

chore(OptionsMenu): deprecate #8798

Merged
merged 10 commits into from Mar 17, 2023

Conversation

gitdallas
Copy link
Contributor

@gitdallas gitdallas commented Mar 9, 2023

What: Closes #8075

https://patternfly-react-pr-8798.surge.sh/components/options-menu

OptionsMenu uses in demos and examples have been commented out until Select next is promoted. We can replace OptionsMenu with the new Select after that goes in. #8073

@patternfly-build
Copy link
Contributor

patternfly-build commented Mar 9, 2023

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

This looks fine to me. But @tlabaj @nicolethoen @evwilkin do we really want to rename the tab "React deprecated." I think we are planning to add a banner and a label to these component pages, correct? I know that is something still being worked out. So also adding "deprecated" to the tab might feel redundant, but welcome you thoughts.

@nicolethoen
Copy link
Contributor

@mcarrano I personally don't think hurts to be redundant, but I see what you mean.
Right now the tab name is set to be React Deprecated for all components moved to the deprecated directory, which is helpful for the components who have a new recommended implementation (without 'deprecated' in the tab), so the added word is helpful in the case of things like the Select and Wizard, for example.

@tlabaj
Copy link
Contributor

tlabaj commented Mar 14, 2023

@mcarrano I agree with @nicolethoen Nicole. I do not think it hurts to call the tab "React deprecated " It will be consistent with other deprecated react components that have a new implementation.

@mcarrano
Copy link
Member

@nicolethoen @tlabaj thanks for your feedback. I went ahead and approved then.

@gitdallas gitdallas marked this pull request as draft March 15, 2023 15:55
Comment on lines -143 to +141
onToggle={(event: any, value: boolean) => {
props.onToggle(value);
onToggle={(event: any) => {
props.onToggle(event);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like this was missed with the onToggle changes

@gitdallas gitdallas force-pushed the chore/deprecate-options-menu branch from 0601e30 to 04ae231 Compare March 16, 2023 13:23
@gitdallas gitdallas marked this pull request as ready for review March 16, 2023 14:29
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Continuing from standup today, don't think there would be any significant difference between getting this in first vs #8072 so I'd be good with making any necessary updates in these files if this gets merged first.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

@tlabaj tlabaj merged commit dea2ecb into patternfly:v5 Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options menu - deprecate options menu
7 participants