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

refact: float content interface #724

Conversation

cartercanedy
Copy link
Contributor

@cartercanedy cartercanedy commented Oct 2, 2024

refactor FloatContent interface for better responsibility delegation

Type of Change

  • Bug fix
  • Refactoring

Description

Testing

Tested by running a few commands, could use some feedback from other users.

Impact

removes replicated code, simplifies interface consumers

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@cartercanedy cartercanedy mentioned this pull request Oct 2, 2024
1 task
@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch from 62427a5 to 7332293 Compare October 2, 2024 01:59
@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

dupe of #723

@cartercanedy
Copy link
Contributor Author

dupe of #723

Not really

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

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

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

also you forgot to add closes #722 to the description

@cartercanedy
Copy link
Contributor Author

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

@cartercanedy
Copy link
Contributor Author

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

@nnyyxxxx
Copy link
Contributor

nnyyxxxx commented Oct 2, 2024

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

never said it wasn't i was just generalizing the fact that it solves the same issue making it a dupe

@cartercanedy
Copy link
Contributor Author

it solves the same issue making it a dupe

The lack of nuance was my concern

@cartercanedy cartercanedy marked this pull request as ready for review October 2, 2024 03:36
@cartercanedy cartercanedy changed the title Refactor float content interface Refactor: float content interface Oct 2, 2024
@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch 2 times, most recently from 3d874c9 to 9e8dc0e Compare October 3, 2024 18:54
@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch from 486b39b to f27852b Compare October 3, 2024 22:37
@cartercanedy
Copy link
Contributor Author

@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

@afonsofrancof
Copy link
Collaborator

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?

@cartercanedy
Copy link
Contributor Author

Totally! Just rebase any changes you made on top of mine and open a PR on my dev branch

@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch from f27852b to 9e8dc0e Compare October 3, 2024 22:54
@afonsofrancof
Copy link
Collaborator

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

cartercanedy#3

@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 4, 2024

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?

@afonsofrancof
Copy link
Collaborator

Will do once I am home :)

@afonsofrancof
Copy link
Collaborator

afonsofrancof commented Oct 4, 2024

@cartercanedy I cleaned it up :)
Let me know if it's good for a rebase

@cartercanedy
Copy link
Contributor Author

@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!

@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch 2 times, most recently from 26a2f11 to 1a131a9 Compare October 10, 2024 23:37
@adamperkowski adamperkowski changed the title Refactor: float content interface refact: float content interface Oct 25, 2024
@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch 9 times, most recently from 51262db to cdf411e Compare October 31, 2024 19:23
@cartercanedy
Copy link
Contributor Author

cartercanedy commented Oct 31, 2024

these changes are just trying rebasing on top of all of the closed PRs, this should be good to go

@adamperkowski
Copy link
Collaborator

these changes are just trying rebasing on top of all of the closed PRs, this should be good to go

#889

@cartercanedy cartercanedy force-pushed the refactor-FloatContent-interface branch from cdf411e to 3d52ef9 Compare October 31, 2024 19:50
@cartercanedy
Copy link
Contributor Author

#889

I figured that cargo.lock got lost in the deps cleanup, thanks for fixing

@cartercanedy
Copy link
Contributor Author

?

@cartercanedy
Copy link
Contributor Author

@ChrisTitusTech any particular reason?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

color blending
5 participants