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(ESSNTL-3726): Add the /groups/%id page (group detail) #1771

Merged

Conversation

gkarat
Copy link
Contributor

@gkarat gkarat commented Feb 21, 2023

Implements https://issues.redhat.com/browse/ESSNTL-3726.
Mocks https://www.sketch.com/s/c19f555b-7887-4423-b55d-575c8dd1dfb7/a/52E5bap.

This adds a new route /groups/%id where id is a group id. The page contains the breadcrumbs, header with the group name or id, and two tabs that contain placeholder (table and further components will be implemented soon). The feature is covered with unit and component tests (the component ones primarily check the right behavior under certain API responses).

How to test

While running the PR locally together with mock server, navigate to /groups/%id with any custom id. The page should render expected content.

Screenshots

image

image

@gkarat gkarat added the enhancement New feature or request label Feb 21, 2023
@gkarat gkarat requested a review from Fewwy February 21, 2023 10:57
@gkarat gkarat self-assigned this Feb 21, 2023
@gkarat gkarat requested a review from a team as a code owner February 21, 2023 10:57
@Fewwy
Copy link
Contributor

Fewwy commented Feb 21, 2023

I think you're missing the group actions dropdown on the top right
https://www.sketch.com/s/c19f555b-7887-4423-b55d-575c8dd1dfb7/a/ygRKL3q

@Fewwy
Copy link
Contributor

Fewwy commented Feb 21, 2023

Other than missing dropdown - page looks good

@gkarat gkarat force-pushed the essntl-3726-group-systems-page-init branch 2 times, most recently from b15ce7c to 1b61129 Compare February 21, 2023 14:18
src/Routes.js Outdated
<Route exact path={routes.groupDetail} component={groupsEnabled
? InventoryGroupDetail
: () => (
<EmptyState>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not this component exist on FEC with an already empty state? If this is the component for 404, it should be there, ready for consumption. Also, maybe we can extract this into some wrapper and avoid repetition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't found the empty state-like components that could replace it. if you could point me out to any would be cool. meanwhile, I extracted this empty state to a separate component in the last commit 👍🏼

@gkarat gkarat force-pushed the essntl-3726-group-systems-page-init branch from 1b61129 to 4bc1f2e Compare February 22, 2023 10:17
@codecov-commenter
Copy link

Codecov Report

Base: 70.85% // Head: 69.40% // Decreases project coverage by -1.45% ⚠️

Coverage data is based on head (f5e9447) compared to base (b80c00e).
Patch coverage: 46.81% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1771      +/-   ##
==========================================
- Coverage   70.85%   69.40%   -1.45%     
==========================================
  Files         108      122      +14     
  Lines        2422     2579     +157     
  Branches      869      885      +16     
==========================================
+ Hits         1716     1790      +74     
- Misses        706      789      +83     
Impacted Files Coverage Δ
...c/components/InventoryDetail/ApplicationDetails.js 0.00% <0.00%> (ø)
src/components/InventoryDetail/DetailHeader.js 0.00% <0.00%> (ø)
src/components/InventoryDetail/InventoryDetail.js 0.00% <0.00%> (ø)
...components/InventoryGroupDetail/GroupDetailInfo.js 0.00% <0.00%> (ø)
...ts/InventoryGroupDetail/InventoryGroupDetail.cy.js 0.00% <0.00%> (ø)
src/components/InventoryGroupDetail/index.js 0.00% <0.00%> (ø)
...ents/InventoryGroups/Modals/DeleteGroupModal.cy.js 0.00% <0.00%> (ø)
...ents/InventoryGroups/Modals/RenameGroupModal.cy.js 0.00% <0.00%> (ø)
src/modules/DetailHeader.js 0.00% <0.00%> (ø)
src/modules/InventoryDetail.js 0.00% <ø> (ø)
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@mkholjuraev mkholjuraev left a comment

Choose a reason for hiding this comment

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

Codewise looks good to me!

@gkarat gkarat merged commit ca24e55 into RedHatInsights:master Feb 22, 2023
gkarat pushed a commit that referenced this pull request Feb 22, 2023
# [1.4.0](v1.3.1...v1.4.0) (2023-02-22)

### Features

* **ESSNTL-3726:** Add the /groups/%id page (group detail) ([#1771](#1771)) ([ca24e55](ca24e55))
@gkarat
Copy link
Contributor Author

gkarat commented Feb 22, 2023

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants