chore(NotificationDrawer):convert examples to typescript #7656
Conversation
|
Preview: https://patternfly-react-pr-7656.surge.sh A11y report: https://patternfly-react-pr-7656-a11y.surge.sh |
That's a great catch. It would be wise that our examples adhere to design guidelines since people like to copy/paste our example code. @mmenestr do you think the design guidelines that Jan references here are up to date and correct?
This is a great question for @mcarrano when he is back from PTO. A quick once over from me did not identify any other differences between the two examples. |
|
The design guidelines for this should be up to date as there have not really been any activity on this for at least the path year. If I recall, the "lightweight" variant was created specifically for OpenShift since they wanted to use a more stripped down implementation of the drawer. |
|
@mcarrano - would Joe C be a good person to connect with for the need to keep (or can) the lightweight example? |
Yes, definitely @janwright73 |
|
@mcarrano @janwright73 @nicolethoen Thats correct, the lightweight example is basically the OpenShift implementation. Since the original design, however, there was a follow on feature to add the x to close the drawer. Also there is no read/unread notifications since we don't have that concept with openshift. I should also say that regrettably we have not yet merged the pr that @DaoDaoNoCode wrote that contributes back the pf component, but thats another story. Design wise, the lightweight example is supposed to reflect the openshift implementation. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Looks good! I just had several (small) changes noted below + one nitpick
packages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerBasic.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerBasic.tsx
Show resolved
Hide resolved
packages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerGroups.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerGroups.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerGroups.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerBasic.tsx
Outdated
Show resolved
Hide resolved
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
|
accepted all suggestions from Eric - please let me know if there are additional tasks to complete this work. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Looks good! The only other thing for me would be updating the "Lightweight" example to more closely resemble what was described as OpenShift's current implementation above.
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
@janwright73 this one can go in once the updates are made to the lightweight example.
|
Hey @tlabaj - the alst update did include the lightweight example updates that Eric requested. Is there an issue with the changes that I missed? |
@janwright73 it looks fine to me. It is however visually different from what was there before. Which is ok based on the conversation above. I will defer to @jcaianirh and @mcarrano for approval on the lightweight example. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Other than Titani's comment above about the lightweight example and getting feedback from Matt and/or Joe, looks good!
|
Since the goal was to match OpenShift's implementation with this example, I'm OK with it even though it diverges from what we have now. That said, I'm not that familiar with the details of OpenShift's implementation so I'll defer to @jcaianirh on whether this is correct. |
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
...ages/react-core/src/components/NotificationDrawer/examples/NotificationDrawerLightweight.tsx
Outdated
Show resolved
Hide resolved
jcaianirh
left a comment
jcaianirh
left a comment
There was a problem hiding this comment.
looks awesome, just a few issues noted in the code review otherwise looks great
…ft usage updates from Joe C
jcaianirh
left a comment
jcaianirh
left a comment
There was a problem hiding this comment.
/lgtm
thanks for making the updates!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
8 participants
What: Closes #7642
Additional issues: I may need some extra guidance here as the existing example is throwing errors on PF.org. I also noticed that the design guidelines stated for grouped examples that only one group should be open at a time. The existing demos do not enforce this behavior. Final question - the 'lightweight' example - what is the purpose of this one? The only difference is the lack of global action menu and no close button for the drawer??