Skip to content

docs: dependent resources and other improvements #1064

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

Merged
merged 9 commits into from
Mar 28, 2022
Merged

docs: dependent resources and other improvements #1064

merged 9 commits into from
Mar 28, 2022

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Mar 22, 2022

No description provided.

@csviri csviri self-assigned this Mar 22, 2022
@csviri csviri requested a review from metacosm March 24, 2022 14:25
@csviri csviri marked this pull request as ready for review March 24, 2022 14:25
}

// omitted code
implements Reconciler<WebPage>, ErrorStatusHandler<WebPage>, EventSourceInitializer<WebPage> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there a way to include code directly from the repo instead of having to copy it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good, point will check!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://emgithub.com/
can do the trick, the problem is that it is not very well formatted (while it is on that page directly), it is not in the resulting jekyll page.
Also I would use permalinks, since any reformat / change can mess this up if we want to use it. Since we reference the code snippet based on line numbers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it messes up the formatting, I was not able to fix it for now. What do you think should we use this? @metacosm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That I already added, see "See the full source code sample here" parts.

Copy link
Collaborator Author

@csviri csviri Mar 25, 2022

Choose a reason for hiding this comment

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

Ahh you mean only that, I think that is very hard to read. At least I personally don't like if it's done that way in the docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if we could embed the code but if we can't do it nicely, then copying the code in the doc is more problematic because then it needs to be maintained and risk being outdated, causing user confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would also prefer that, it's just not formatted now :(
And was not able to fix it for now.

I would merge this as it is, to be consistent with other parts of the docs, and we can migrate / take a look on the embedding & formatting isses seratelly, pretty sure that can be fixed somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And will migrate all the docs to that in a separate PR.

Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@csviri csviri merged commit 5b1e333 into main Mar 28, 2022
@csviri csviri deleted the docs branch March 28, 2022 14:16
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