Skip to content

Conversation

lucasgoral
Copy link
Contributor

@lucasgoral lucasgoral commented Apr 15, 2025

What this PR does / why we need it:

Added a feature that displays YAML files for projects ,namespaces and MCPs.

@lucasgoral lucasgoral marked this pull request as ready for review April 23, 2025 12:09
@andreaskienle andreaskienle self-assigned this Apr 23, 2025
Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

Nice work! It looks really good, and I like how you designed the dialog, especially putting the ⁠isOpen state inside the button component and wrapping the YAML display inside the loader.

I also think the react-syntax-highlighter package is a great choice. I'm only slightly concerned about the bundle size. Since we don't have plans to support mobile users right now, I think it's negligible and we can proceed with this implementation. There's also a light build option (or alternatively prism-react-renderer) that we could keep in mind for future optimization.

I found some minor issues below.

import { useTranslation } from 'react-i18next';

export type ResourceProps = {
workspaceName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this ever be undefined?

Just wondering if we can simply write

Suggested change
workspaceName?: string;
workspaceName: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for the Project YAML it is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, in this case I’m wondering what if we made this a bit more generic by decoupling from the workspace/project context? Something like:

export type YamlViewButtonProps = {
  apiPath: string;
  localFilename: string;
}

After all, it’s essentially just a dialog that displays downloads and YAML content. It probably shouldn’t need to know the specifics of how to retrieve that data.

Just thinking out loud here - we’ll likely need to revisit the fetch logic when we implement GraphQL anyway, so we can see how this approach works then.

const [isOpen, setIsOpen] = useState(false);
const { t } = useTranslation();
return (
<span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for the <span>?

Suggested change
<span>
<>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is wrapped in span so it it display: inline instead of display:block. So I can put it next to some text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn’t <> instead of <span> put the UI5 button (<Button>) directly to the top level which has display:inline-block? Anyway, not a big deal, was just wondering.

@lucasgoral
Copy link
Contributor Author

Nice work! It looks really good, and I like how you designed the dialog, especially putting the ⁠isOpen state inside the button component and wrapping the YAML display inside the loader.

I also think the react-syntax-highlighter package is a great choice. I'm only slightly concerned about the bundle size. Since we don't have plans to support mobile users right now, I think it's negligible and we can proceed with this implementation. There's also a light build option (or alternatively prism-react-renderer) that we could keep in mind for future optimization.

I found some minor issues below.

Thank you. Yes in the future we can easily replace this package if smaller bundle size is needed.

@lucasgoral
Copy link
Contributor Author

With the separate task I will:

  • add MCP's resources preview
  • remove horizontal screen scrollbar for YAML view
  • remove managedFields property from the YAML object
  • extract download function as a separate hook

andreaskienle
andreaskienle previously approved these changes Apr 28, 2025
Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

Looks good!

I added some comment below but we can address the remaining items in a follow-up PR. 👍

@@ -0,0 +1,33 @@
import { YamlViewButtonProps } from './YamlViewButton.tsx';
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused import

Suggested change
import { YamlViewButtonProps } from './YamlViewButton.tsx';

import { useTranslation } from 'react-i18next';

export type ResourceProps = {
workspaceName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, in this case I’m wondering what if we made this a bit more generic by decoupling from the workspace/project context? Something like:

export type YamlViewButtonProps = {
  apiPath: string;
  localFilename: string;
}

After all, it’s essentially just a dialog that displays downloads and YAML content. It probably shouldn’t need to know the specifics of how to retrieve that data.

Just thinking out loud here - we’ll likely need to revisit the fetch logic when we implement GraphQL anyway, so we can see how this approach works then.

const [isOpen, setIsOpen] = useState(false);
const { t } = useTranslation();
return (
<span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn’t <> instead of <span> put the UI5 button (<Button>) directly to the top level which has display:inline-block? Anyway, not a big deal, was just wondering.

andreaskienle
andreaskienle previously approved these changes Apr 28, 2025
@lucasgoral lucasgoral merged commit d4fd7cb into main Apr 28, 2025
4 checks passed
@lucasgoral lucasgoral deleted the feature/display-resource-yaml branch April 28, 2025 09:53
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.

3 participants