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

don't overwrite background when inheriting #69

Merged
merged 3 commits into from Feb 25, 2022

Conversation

76creates
Copy link
Contributor

It's contra intuitive that backgroundKey is the only key that is always inherited.

@meowgorithm
Copy link
Member

Thanks for this! Would you mind posting a tiny bit of sample code to help explain why this makes sense? I know it might seem trivial, but it'll help expedite this.

Thanks!

@76creates
Copy link
Contributor Author

Thanks for this! Would you mind posting a tiny bit of sample code to help explain why this makes sense? I know it might seem trivial, but it'll help expedite this.

Thanks!

From the function point of view Inherit, and any other fn, should be as consistent as possible, meaning that we should minimise special cases like those with background and margin background keys as they might lead to difficult to debug situations. This is my motive for this PR.

From the user point of view, in my case that is, I was looking for a way to set a default style for an array of Style objects, we can normally achieve that using Copy but in this case both the inheritor and origin objects mutate over time and are "joined" at one moment. I can go around this issue by checking if background is set, storing it if it is, then inheriting, and then overwriting the background, which is all a bit messy in my opinion.

@meowgorithm
Copy link
Member

Cool, so this makes sense. Basically if a background color is set on the inheritor it shouldn’t be overridden during inheritance. Clearly just something that was overlooked.

We’ll still need to vet this PR before merging, so if you have any sample code it’ll help expedite the process.

@76creates
Copy link
Contributor Author

Cool, so this makes sense. Basically if a background color is set on the inheritor it shouldn’t be overridden during inheritance. Clearly just something that was overlooked.

We’ll still need to vet this PR before merging, so if you have any sample code it’ll help expedite the process.

https://gist.github.com/76creates/8f529f2b975f3cd6dee1d2a70260d53e

@76creates
Copy link
Contributor Author

for full example check out https://github.com/76creates/stickers
The problem appears here

@muesli
Copy link
Member

muesli commented Feb 23, 2022

for full example check out https://github.com/76creates/stickers The problem appears here

🤯 Nice work on stickers!

@meowgorithm meowgorithm merged commit 3616f64 into charmbracelet:master Feb 25, 2022
@meowgorithm
Copy link
Member

Thanks again for this. It's awesome seeing some good use of inheritance on stickers.

@76creates
Copy link
Contributor Author

Thanks again for this. It's awesome seeing some good use of inheritance on stickers.

no, no, thank you folks for making such an awesome lib to begin with ⚡

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.

None yet

3 participants