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

refactor: add and use XAboutCard component in detail views #3300

Merged
merged 16 commits into from
Dec 17, 2024

Conversation

schogges
Copy link
Contributor

@schogges schogges commented Dec 13, 2024

These changes introduce a new global component XAboutCard that is used to improve the layout of the formerly used KCard in the detail views "About" section. It is based on the AppAboutSection exported by @kong-ui-public/app-layout. The layout in the content slot changed to a horizontal layout with badges (XBadge) instead of the column based layout for a general improvement of readability. All XAboutSection show the date times of creation and potential modification of the resource.

image

For better visual separation of the ResourceStatus entries in the ControlPlaneStatus I have introduced a new class .columns-with-borders which works similarly to .stack-with-borders but for a horizontal layout.

Screenshot 2024-12-13 at 18 28 17

Furthermore I have replaced several occurrences of class="column" or class="stack" in the files I touched with XLayout and replaced static text with i18n.

@schogges schogges requested a review from a team as a code owner December 13, 2024 17:37
@schogges schogges requested review from johncowen and removed request for a team December 13, 2024 17:37
Copy link

netlify bot commented Dec 13, 2024

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit bf9109e
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/67619a2427ecd900083e642e
😎 Deploy Preview https://deploy-preview-3300--kuma-gui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@schogges schogges force-pushed the feat/add_x-app-about-section branch from 06a1e2c to 6e8fa1c Compare December 13, 2024 17:37
@schogges schogges changed the title Feat/add x app about section refactor: add and use XAboutSection component in detail views Dec 13, 2024
@schogges schogges force-pushed the feat/add_x-app-about-section branch from 6e8fa1c to b820afc Compare December 13, 2024 17:46
@johncowen
Copy link
Contributor

Would it be much effort to split this PR in two:

  1. a PR with the changes related to the innards of AppAboutSection
  2. a PR containing nothing but the re-introduction of AppAboutSection itself.

The reason I'm checking is, if its not too much effort it would be good to do the AppAboutSection as a second isolated PR, mostly because we have very recently flip-flopped between using this or not using this. I'd like it if we explicitly made a PR that does this and explains the reasoning behind the flip-flop and almost acts as a mini-ADR. We then have that for a clean reference for wants to know the reasons for the "flip" back.

Also there are a couple of things that I'd like to do with the design of XAppAboutSection (seeing as we have that to wrap over AppAboutSection), that I'd like to pass by you so it would be good to have a separate PR to do that. lmk!

@schogges
Copy link
Contributor Author

Would it be much effort to split this PR in two:

  1. a PR with the changes related to the innards of AppAboutSection
  2. a PR containing nothing but the re-introduction of AppAboutSection itself.

The reason I'm checking is, if its not too much effort it would be good to do the AppAboutSection as a second isolated PR, mostly because we have very recently flip-flopped between using this or not using this. I'd like it if we explicitly made a PR that does this and explains the reasoning behind the flip-flop and almost acts as a mini-ADR. We then have that for a clean reference for wants to know the reasons for the "flip" back.

Also there are a couple of things that I'd like to do with the design of XAppAboutSection (seeing as we have that to wrap over AppAboutSection), that I'd like to pass by you so it would be good to have a separate PR to do that. lmk!

It takes some efforts, but I think I can reduce it to a minimum when I keep the XAboutSection component, but replace the internal reference to AppAboutSection with an own implementation using KCard and XTimespan (just what we used to do before).

@johncowen
Copy link
Contributor

Ok, I understood that you are going to keep the XAppAboutSection and its innards will temporarily contain KCard and XTimespan?

I guess thats mixing concerns still a little. The reason ideally we'd have it all in one PR is so that would mean the re-introduction of XAppAboutSection (or whatever we end up calling it) and its implementation is in one single PR that we can point/link to and not mixed in with other work. Ideally it would all be contained in one PR.

If XAppAboutSection can't all go in its own PR, I don't think there a massive point in splitting, it would be splitting for the sake of making two PRs rather than splitting for the sake of clarifying things and having a single PR to point/link to. lmk either way and I can start reviewing this PR or not.

@schogges
Copy link
Contributor Author

schogges commented Dec 16, 2024

Ok, I understood that you are going to keep the XAppAboutSection and its innards will temporarily contain KCard and XTimespan?

I guess thats mixing concerns still a little. The reason ideally we'd have it all in one PR is so that would mean the re-introduction of XAppAboutSection (or whatever we end up calling it) and its implementation is in one single PR that we can point/link to and not mixed in with other work. Ideally it would all be contained in one PR.

If XAppAboutSection can't all go in its own PR, I don't think there a massive point in splitting, it would be splitting for the sake of making two PRs rather than splitting for the sake of clarifying things and having a single PR to point/link to. lmk either way and I can start reviewing this PR or not.

The main point of adding XAboutSection is to add some stylings (e.g. for the DescriptionCard). This would still be required when using KCard and XTimestamp instead, because I would not like to repeat that in all the places where DescriptionCard is being used. So regardless of using AppAboutSection as a base, I would want to add a component.

The styles could also be added to the DescriptionCard directly, but this would also affect the look and feel in other places like *SummaryView.

@johncowen
Copy link
Contributor

Ok gotcha, let me have a quick look at this here. I'm curious as to why there is styling needed for DescriptionCard outside of the component itself, gimme a sec to look over this in more detail. I think I might have misunderstood how inter-twined (or not) this is.

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

I made an initial pass on this, mostly concentrating on XAboutAppSection. Lmk know on those comments, I'd be keen to get this work in by tomorrow/Wednesday preferably 👍

I was about to look and see why we have to overwrite things and others don't but wondered if you'd checked that before doing this? lmk if so, if not I'll take look.

@johncowen
Copy link
Contributor

johncowen commented Dec 17, 2024

I just took this for a spin, its actually surprisingly better than I thought using this component would be (i.e. the dates are on the same baseline as the titles here)

Couple of things I had questions on:

Screenshot 2024-12-17 at 11 55 42

^ mTLS looks like the odd one out here now that we only use the old style for numeric stats, wdyt, any suggestions?

Screenshot 2024-12-17 at 11 59 10

We chatted a bit offline about this one ^, I'm just trying to think of a better solution to this. It's really weird that in one place mTLS is the odd one out amongst stats and here the stats is the odd one out amongst other kv's. Did you have any other ideas?

schogges added a commit that referenced this pull request Dec 17, 2024
We recently noticed that `console` statements are not catched in `.vue`
files inside the `template` (although catched in `script`) (xref:
#3300 (comment)).

---------

Signed-off-by: schogges <moritz.fleck@konghq.com>
@schogges
Copy link
Contributor Author

Couple of things I had questions on:

Screenshot 2024-12-17 at 11 55 42 ^ `mTLS` looks like the odd one out here now that we only use the old style for numeric stats, wdyt, any suggestions? Screenshot 2024-12-17 at 11 59 10 We chatted a bit offline about this one ^, I'm just trying to think of a better solution to this. It's really weird that in one place mTLS is the odd one out amongst stats and here the stats is the odd one out amongst other kv's. Did you have any other ideas?

Yeah, this is tricky. I am not really sure what the best solution is. Ideally we can get rid of the mixture. At least for the mTLS one we could probably move it to the AboutSection?

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

Couple more bits, sorry this is a bit piecemeal, I think we should be able to get this in by the end of today which would be great. There are a couple of things here that I'm not sure are outstanding or not.

@johncowen
Copy link
Contributor

Yeah, this is tricky. I am not really sure what the best solution is. Ideally we can get rid of the mixture. At least for the mTLS one we could probably move it to the AboutSection?

Yeah I think that might be best, give it a try and see what we think? We can always revert. Not sure about the DataPlane one those, thats unfortunate, not quite sure 🤔

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

Ok I think I've covered everything, there are a couple of outstanding things that we just want to cover off either way. But wanted to note that I don't think theres anything else here.

Appreciate I had quite a few comments and concerns, I was being extra careful considering everything. Also appreciate how you dealt with my comments with care and patience, so thanks! We've both said ideally we don't want to be coming back here again at any point soon, if at all, and I think this is in the best state we can get it now 🎉

Just waiting to see if there's anything else to be done on the unresolved comments, then I can come back here with a 👍 I think

Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
Signed-off-by: schogges <moritz.fleck@konghq.com>
@schogges schogges force-pushed the feat/add_x-app-about-section branch from e871116 to bf9109e Compare December 17, 2024 15:34
@schogges schogges changed the title refactor: add and use XAboutSection component in detail views refactor: add and use XAboutCard component in detail views Dec 17, 2024
@schogges
Copy link
Contributor Author

Ok I think I've covered everything, there are a couple of outstanding things that we just want to cover off either way. But wanted to note that I don't think theres anything else here.

Just pushed the last changes and resolved some conflicts. I think it's ready to go now 🙂

Appreciate I had quite a few comments and concerns, I was being extra careful considering everything. Also appreciate how you dealt with my comments with care and patience, so thanks! We've both said ideally we don't want to be coming back here again at any point soon, if at all, and I think this is in the best state we can get it now 🎉

Just waiting to see if there's anything else to be done on the unresolved comments, then I can come back here with a 👍 I think

Thanks a lot for your thoughts, comments and patience 🙂 this is much better now! 🙌

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

Ok lets get this in!

@schogges schogges merged commit 5d05f5b into kumahq:master Dec 17, 2024
16 checks passed
@schogges schogges deleted the feat/add_x-app-about-section branch December 17, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants