linutil
linutil copied to clipboard
ChrisTitusTech
Refactor: float content interface
refactor FloatContent interface for better responsibility delegation
Type of Change
- [x] Bug fix
- [x] Refactoring
Description
Testing
Tested by running a few commands, could use some feedback from other users.
Impact
removes replicated code, simplifies interface consumers
- Resolves #722
Checklist
- [x] My code adheres to the coding and style guidelines of the project.
- [x] I have performed a self-review of my own code.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [x] I have made corresponding changes to the documentation.
- [x] My changes generate no errors/warnings/merge conflicts.
dupe of #723
Not really
both of these PRs solve the same color bleeding issue, except yours comes later with a different proposed fix to the same issue. So yes this is a duplicate of my PR
This is more than just a fix for the styling issues, this makes the FloatContent interface more reasonable in terms of formatting and uniform styling of all Floats
yours comes later with a different proposed fix to the same issue
It coincidentally fixes the same issue, but also refactors, so it's not a complete duplicate. The issue can be solved by your pr while also leaving this pr valid
This is more than just a fix for the styling issues, this makes the
FloatContentinterface more reasonable in terms of formatting and uniform styling of allFloats
never said it wasn't i was just generalizing the fact that it solves the same issue making it a dupe
it solves the same issue making it a dupe
The lack of nuance was my concern
@afonsofrancof Just implemented what you did in #633 (I made sure to give you co-authorship) Please take a look and let me know if I captured what you were hoping to implement in your PR. I'm happy to update if I missed something
Hey carter! I like what you did, but the main goal of my PR was to remove the FloatingTextMode so that we could display anything we want instead of being fixed to those 3 enum types. What I did would make it so that we could make the titles generic for any float we want to create in the future. What do you think? Maybe we could add it here as well?
Totally! Just rebase any changes you made on top of mine and open a PR on my dev branch
In August, when I was creating the FloatingContent interface and the FloatingText stucts, what I had in mind was that FloatingContent could be implemented by anything that wanted to show a float.
I think that restricting what the FloatingText can have as a title with an Enum goes against being generic. I think we should keep FloatingText as a way to show a generic float with any text and any title. We could expand upon the FloatingText struct, later on, if we wanted to have different behavior depending on the type of FloatingText we are creating (Preview,Descriptio, etc.) , while leaving FloatingText alone.
That is why the title is now a single string instead of having a "name" field. We don't know if the "name" value will even make sense for future FloatingTexts.
Let me know what you think
https://github.com/cartercanedy/linutil/pull/3
I really like the changes. Only thing preventing the rebase pull is the temp-dir patch being out-of-order with upstream and your changes being applied before mine. Do you mind dropping Jeev's patch and applying your changes on top of mine to make the rebase possible?
@cartercanedy I cleaned it up :) Let me know if it's good for a rebase
@afonsofrancof I spent way too much time trying to linearize the tree, but I'll just let your first commit stick around as a merge. Thanks for the help!
these changes are just trying rebasing on top of all of the closed PRs, this should be good to go
these changes are just trying rebasing on top of all of the closed PRs, this should be good to go
#889
#889
I figured that cargo.lock got lost in the deps cleanup, thanks for fixing