Skip to content

chore(ApplicationLauncher): deprecate #8836

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.

Sign up for GitHub

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

Jump to bottom
Merged
tlabaj merged 2 commits into from
Mar 27, 2023
Merged

chore(ApplicationLauncher): deprecate #8836

tlabaj merged 2 commits into from
Mar 27, 2023

Conversation

@gitdallas
Copy link
Contributor

@gitdallas gitdallas commented Mar 20, 2023 •
edited
Loading

@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 20, 2023 •
edited
Loading

Preview: https://patternfly-react-pr-8836.surge.sh

A11y report: https://patternfly-react-pr-8836-a11y.surge.sh

thatblindgeye
thatblindgeye requested changes Mar 21, 2023
View reviewed changes
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.

The comments below will apply to the other demos that were updated to use the composable menu implementation of an app launcher (the MastheadWithUtilities.... demo and all 4 Page demos I believe)

@gitdallas gitdallas force-pushed the chore/deprecate-app-launcher branch from b772690 to 1dd6acb Compare March 21, 2023 15:20
@gitdallas
Copy link
Contributor Author

gitdallas commented Mar 21, 2023

I wonder if I should import DashboardHeader into these to reduce all the duplicate code? The possible reason not to would be... "In the example, we want to show the full code that goes into the Page component."

Thoughts @nicolethoen @thatblindgeye @tlabaj ?

@tlabaj tlabaj requested review from mcarrano and mmenestr March 21, 2023 16:10
@gitdallas gitdallas requested a review from thatblindgeye March 21, 2023 17:38
@gitdallas gitdallas changed the title Chore/deprecate app launcher chore(ApplicationLauncher): deprecate Mar 21, 2023
thatblindgeye
thatblindgeye approved these changes Mar 21, 2023
View reviewed changes
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.

Keeping the examples as-is rather than importing DashboardHeader makes sense from a "show how someone else can implement the same thing" sort of way. #7927 was an update to the Page demos that specifically did not import the Dashboard wrapper at least

nicolethoen
nicolethoen reviewed Mar 22, 2023
View reviewed changes
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

This all looks like it works. Is there a reason that our examples/demos shouldn't or can't use a Dropdown-next to replace the old application launchers?

@gitdallas
Copy link
Contributor Author

gitdallas commented Mar 23, 2023 •
edited
Loading

@nicolethoen

I choose the replacement based on the issue description "Update any demos/examples that use the application launcher to use a recommended solution - the composable menu demo."

@nicolethoen
Copy link
Contributor

nicolethoen commented Mar 23, 2023

It may be worth doing once the dropdown-next is moved out of the dropdown directory. But i think this is fine for now. I'm going to open a follow up issue.

@thatblindgeye thatblindgeye mentioned this pull request Mar 23, 2023
Closed
tlabaj
tlabaj approved these changes Mar 23, 2023
View reviewed changes
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!
I will defer to @mcarrano on wether the demos should be using the Dropdown next, or be built with composable components as they are in this PR.

@nicolethoen
Copy link
Contributor

nicolethoen commented Mar 23, 2023

I'd argue that if we are able to build any of our deprecated components as they function in demos/examples today using next components, we should (since they will all be promoted with this release) so that products see how to build things out of the box and not unnecessarily try to compose things completely on their own. There are a lot of interaction details to make sure are wired up correctly when they are built completely composably.

I'm working on opening an issue that is keeping track of all the places this might apply.

@tlabaj
Copy link
Contributor

tlabaj commented Mar 23, 2023

I'd argue that if we are able to build any of our deprecated components as they function in demos/examples today using next components, we should (since they will all be promoted with this release) so that products see how to build things out of the box and not unnecessarily try to compose things completely on their own. There are a lot of interaction details to make sure are wired up correctly when they are built completely composably.

I'm working on opening an issue that is keeping track of all the places this might apply.

I agree, if it was wired up, that would be preferable. But the simple dropdown does not have all the functionality an application launcher may need as seen in the demo. That is why I am interested in what @mcarrano thinks here. https://patternfly-react-v5.surge.sh/demos/composable-menu#composable-application-launcher

tlabaj
tlabaj requested changes Mar 24, 2023
View reviewed changes
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.

@gitdallas We discussed this PR at the leads meeting today and we came to the consensus that we only need to demo the application launcher in the Masthead demo. That demo should mirror the features we demonstrate in composable application launcher demo since that is most likely how it is being used in product.

You can remove the application launcher from the other demos.

@gitdallas gitdallas force-pushed the chore/deprecate-app-launcher branch 2 times, most recently from 5bf9a46 to 0035708 Compare March 24, 2023 19:35
@nicolethoen
Copy link
Contributor

nicolethoen commented Mar 24, 2023 •
edited
Loading

Looks to me that the search in the Masthead demo's App Launcher replacement is filtering out every option every time

@gitdallas
Copy link
Contributor Author

gitdallas commented Mar 24, 2023 •
edited
Loading

Looks to me that the search in the Masthead demo's App Launcher replacement is filtering out every option every time

@nicolethoen looks like that is happening in v5 generally in the composable menu example as well. i saw it was happening locally before my changes, also seen in a surge for a different pr https://patternfly-react-pr-8864.surge.sh/demos/composable-menu

i had assumed it just wasn't set up to work, only now did i check pf4 and see it is working there

i'm about to head out of town for the weekend but i can see if i can check what's going on when i get where i'm going.. if we want that fix as part of this pr

@gitdallas gitdallas requested review from nicolethoen and tlabaj March 24, 2023 21:39
@tlabaj
Copy link
Contributor

tlabaj commented Mar 24, 2023

i'm about to head out of town for the weekend but i can see if i can check what's going on when i get where i'm going.. if we want that fix as part of this pr

We will need to open an issue for the composable menu demo so we can probably fix it with that if @nicolethoen agrees.

tlabaj
tlabaj approved these changes Mar 24, 2023
View reviewed changes
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

@gitdallas
Copy link
Contributor Author

gitdallas commented Mar 25, 2023

@nicolethoen @tlabaj - i pushed a quick fix for the filter issue

@gitdallas
chore(ApplicationLauncher): deprecate
70cad1c 
converting demos

add import in md

remove applauncher from pagestickysectionbreadcrumb example

update examples after deprecating app launcher

 import

missing semi colon

address pr feedback

remove applauncher from page/dashboard demos

use composablemenu applauncher demo in masthead demo

rebase markers

rebase fix

rebase fix

fix test snap after rebase

fixing dropdown imports after rebase

fix applauncher search
@gitdallas gitdallas force-pushed the chore/deprecate-app-launcher branch from 090f693 to 70cad1c Compare March 27, 2023 15:26
mcarrano
mcarrano approved these changes Mar 27, 2023
View reviewed changes
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.

Looks great. Thanks @gitdallas !

@tlabaj tlabaj merged commit 957aec7 into patternfly:v5 Mar 27, 2023
@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 27, 2023

Your changes have been released in:

  • @patternfly/react-code-editor@5.0.0-alpha.51
  • @patternfly/react-core@5.0.0-alpha.50
  • @patternfly/react-docs@6.0.0-alpha.55
  • demo-app-ts@5.0.0-alpha.34
  • @patternfly/react-integration@5.0.0-alpha.15
  • @patternfly/react-table@5.0.0-alpha.51

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolethoen nicolethoen nicolethoen approved these changes

@tlabaj tlabaj tlabaj approved these changes

@thatblindgeye thatblindgeye thatblindgeye approved these changes

@mmenestr mmenestr Awaiting requested review from mmenestr

+1 more reviewer

@mcarrano mcarrano mcarrano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Application launcher - deprecate application launcher

6 participants

@gitdallas @patternfly-build @nicolethoen @tlabaj @mcarrano @thatblindgeye