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.

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

Tornjak dashboard details metadata and UI [Part2] #18

Merged
merged 19 commits into from
Sep 16, 2021
Merged

Tornjak dashboard details metadata and UI [Part2] #18

merged 19 commits into from
Sep 16, 2021

Conversation

mamy-CS
Copy link
Collaborator

@mamy-CS mamy-CS commented Aug 23, 2021 •
edited
Loading

Moving - lumjjb/tornjak-old#95
Tornjak Details page
#6
Signed-off-by: Mohammed Abdi mohammed.munir.abdi@ibm.com

tornjak dashboard details repo move to publuc with new commits
4bb0b62 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
lumjjb added a commit that referenced this pull request Aug 24, 2021
@lumjjb
Merge pull request #18 from mamy-CS/ui-list-category-click-refresh
ac45aab 
UI list category click refresh
lumjjb
lumjjb requested changes Aug 24, 2021
View reviewed changes
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Thanks @mamy-CS , feature is awesome, tested and looks and feels great! Have couple comments on code changes, some error handling, code reuse and a couple nits.

tornjak-frontend/src/App.js Show resolved Hide resolved
tornjak-frontend/src/components/dashboard/dashboard-details.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/dashboard/dashboard-details.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/dashboard/entries-dashboard-table.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/dashboard/entries-dashboard-table.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/spiffe-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/spiffe-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/spiffe-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Show resolved Hide resolved
@lumjjb
Copy link
Collaborator

lumjjb commented Aug 24, 2021

Testing had a console error

index.js:1 Warning: Failed prop type: Invalid prop `modifiers` of type `array` supplied to `ForwardRef(Popper)`, expected `object`.

Can you help verify if this is because of pre-materia upgrade to the latest version like the other errors ?

mamy-CS
mamy-CS commented Aug 25, 2021
View reviewed changes
Copy link
Collaborator Author

@mamy-CS mamy-CS left a comment

Choose a reason for hiding this comment

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

Warning: Failed prop type: Invalid prop modifiers of type array supplied to ForwardRef(Popper), expected object.
Yes, this warning is because of Datagrid version, warning removed in V5.

Mohammed Abdi added 3 commits August 25, 2021 10:26
nits first round
3cf3a40 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
nit part 2
04bfc9a 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
update comment
a9a007d 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
lumjjb
lumjjb reviewed Aug 26, 2021
View reviewed changes
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Couple suggestions on naming / reuse and questions about making certain functions private in tornjak-helper.js

tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
tornjak-frontend/src/components/tornjak-helper.js Outdated Show resolved Hide resolved
nits part 3
25e810b 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Mohammed Abdi added 2 commits August 27, 2021 16:04
nit part 4
e486f6d 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
agent entries
f46a4a7 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
nit part 5
1edc2f3 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
lumjjb
lumjjb previously approved these changes Sep 1, 2021
View reviewed changes
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

LGTM, 1 minor nit.

tornjak-frontend/src/components/dashboard/entries-dashboard-table.js Outdated Show resolved Hide resolved
@lumjjb
Copy link
Collaborator

lumjjb commented Sep 1, 2021

code lgtm, need some additional comments for structures defined at top of tornjak-helpers. I will merge when im back from PTO next week after the additional nits/comments.

Mohammed Abdi added 2 commits September 1, 2021 15:18
nit part 6
e9eeeb4 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
change isAdmin flag on entries table to check mark
6351db4 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
@lumjjb
Copy link
Collaborator

lumjjb commented Sep 9, 2021 •
edited
Loading

@mamy-CS Selected details on agents doesn't seem to work for me, can you check that piece of code?

Direct hits on cluster details page also doesnt work for me (it works fine for entries)
i.e. http://localhost:10000/tornjak/dashboard/details/clusters/qw

mamy-CS
mamy-CS commented Sep 9, 2021
View reviewed changes
Copy link
Collaborator Author

@mamy-CS mamy-CS left a comment

Choose a reason for hiding this comment

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

Works here. Both clusters and Agents including entries.
Screen Shot 2021-09-09 at 1 40 40 PM

@lumjjb
Copy link
Collaborator

lumjjb commented Sep 9, 2021

For clusters, the error message i got is

react-dom.production.min.js:209 TypeError: Cannot read properties of undefined (reading 'selectors')
    at a.value (spiffe-helper.js:91)
    at a.value (spiffe-helper.js:190)
    at a.value (tornjak-helper.js:61)
    at tornjak-helper.js:72
    at Array.map (<anonymous>)
    at a.value (tornjak-helper.js:71)
    at a.value (tornjak-helper.js:88)
    at a.value (tornjak-helper.js:37)
    at a.value (dashboard-details-render.js:64)
    at Va (react-dom.production.min.js:182)

For the agent, the issue seems to be that whenever, there is a // within the URL it gets translated to a single /. Perhaps it would be better to encode it as a GET parameter so that it gets URL encoded properly.

@lumjjb lumjjb self-requested a review September 13, 2021 14:06
Mohammed Abdi added 2 commits September 13, 2021 10:35
updating d3 package
eef8531 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
edge case for clusters dashboard
93a740a 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
add query strings
8131bbc 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Mohammed Abdi and others added 2 commits September 14, 2021 12:21
takeing care of empty selectors
e5bf4fc 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
@mamy-CS
Merge branch 'main' into tornjak-dashboard-ui-part2
5195029
@lumjjb
Copy link
Collaborator

lumjjb commented Sep 15, 2021

Can you take a run through the file as well to find the similar instances of use selectors.map and do the checking there as well.

Revert "Merge branch 'main' into tornjak-dashboard-ui-part2"
d909435 
This reverts commit 5195029, reversing
changes made to e5bf4fc.

Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
Mohammed Abdi added 3 commits September 15, 2021 11:20
checking for empty selectors
c3f44a8 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
clusters details not working when opened in new link fixed
091caa1 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
fix dashboard table details title click bug
2b10c6b 
Signed-off-by: Mohammed Abdi <mohammed.munir.abdi@ibm.com>
lumjjb
lumjjb approved these changes Sep 16, 2021
View reviewed changes
Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for helping to fix the issues that came up

@lumjjb lumjjb merged commit 21d9105 into spiffe:main Sep 16, 2021
lumjjb added a commit that referenced this pull request Sep 17, 2021
@lumjjb
Revert "Tornjak dashboard details metadata and UI [Part2] (#18)"
bbdf6b8 
This reverts commit 21d9105.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lumjjb lumjjb lumjjb approved these changes

@mrsabath mrsabath Awaiting requested review from mrsabath mrsabath is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mamy-CS @lumjjb