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 common code for logs pages #110

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Refactor common code for logs pages #110

merged 1 commit into from
Oct 9, 2023

Conversation

smitpatel
Copy link
Contributor

So we don't have to worry about aligning things on 3 pages.

Copy link
Member

@tlmii tlmii left a comment

Choose a reason for hiding this comment

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

I think we could go a little further here and make the ResourceLogs component be a base class for the other three, and have the other three "render" using

@{ base.BuildRenderTree(__builder); }

And make similar changes that I mentioned in comments in #113 about changing the Funcs to just be normal method overrides (along with changing the [Parameters] to just be protected abstract properties).

BUT I think this is way better than it was. So if you want to go ahead and get this in, it does seem to work well. I can do the above suggestions and send out a PR and see what you think.

@smitpatel
Copy link
Contributor Author

@{ base.BuildRenderTree(__builder); }

Will try this. I think the main reason I ended up this PR with component was due to rendering. Other PR, base class provides functionality to help render, here render itself was movable to base. I didn't know about base rendering.

@smitpatel
Copy link
Contributor Author

@tlmii - Updated. Tagging in case you want to have another look. Marking as auto-complete.

@smitpatel smitpatel enabled auto-merge (squash) October 9, 2023 18:55
@smitpatel smitpatel enabled auto-merge (squash) October 9, 2023 19:06
@smitpatel smitpatel merged commit 91e92b9 into main Oct 9, 2023
4 checks passed
@smitpatel smitpatel deleted the smit/common branch October 9, 2023 19:16
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants