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

Implement Renderable for strings and numbers #1512

Closed
wants to merge 1 commit into from
Closed

Implement Renderable for strings and numbers #1512

wants to merge 1 commit into from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 21, 2020 •
edited
Loading

Description

That small change add implementation of Renderable trait for String, str, iXX, uXX, fXX and bool. This is useful, if you have generic component which required Renderable fields, which can be just some strings or numbers.

Unfortunately, generic implementation for all ToString types is impossible, because it conflict with impl Renderable for Component. This would not have happened if Component required Renderable:

trait Renderable { ... }
trait Component: Renderable { ... }

Why approach with impl Renderable for Component was chosen?

Checklist:

  • I have run ./ci/run_stable_checks.sh
  • I have reviewed my own code
  • I have added tests

@siku2
Copy link
Member

siku2 commented Aug 21, 2020 •
edited
Loading

@jstarry, do we even still want to keep the Renderable trait around? The scheduler doesn't need it and it seems like a pointless abstraction over Into<Html> which is used everywhere else. Into<Html> already covers the implementation proposed here as it has a blanket implementation for the Display trait.
When Renderable was introduced in #91 it was responsible for rendering components, but today everything is included in the Component trait.

I just created a branch for the required changes which is ready to go (diff: master...siku2:remove-renderable) but I'd like to hear your thoughts first

@jstarry
Copy link
Member

jstarry commented Aug 22, 2020

I'm happy to remove Renderable!

@siku2
Copy link
Member

siku2 commented Aug 22, 2020

Out of interest, how are you currently using the Renderable trait in your components, @Mingun?

@Mingun
Copy link
Contributor Author

Mingun commented Aug 22, 2020

I'm working on tree-table component and I planned to use it as trait bound for Cell type in a tree. But now I think, that this complicates things a lot, so I'm decide to just return Html from tree model for cell data. If you think, that Renderable must gone, I'll not object.

@siku2 siku2 mentioned this pull request Aug 22, 2020
Merged
2 tasks
@siku2
Copy link
Member

siku2 commented Aug 22, 2020

Alright, closing this in favour of #1517. I really do appreciate that you took the time to open this PR, @Mingun - even if I kind of shot it down 😓

@siku2 siku2 closed this Aug 22, 2020
@Mingun Mingun deleted the renderable-primitives branch August 22, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mingun @siku2 @jstarry