Skip to content

don't overwrite background when inheriting#69

Merged
meowgorithm merged 3 commits intocharmbracelet:masterfrom
76creates:master
Feb 25, 2022
Merged

don't overwrite background when inheriting#69
meowgorithm merged 3 commits intocharmbracelet:masterfrom
76creates:master

Conversation

@76creates
Copy link
Copy Markdown
Contributor

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

@meowgorithm
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@muesli
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

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

@76creates
Copy link
Copy Markdown
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.

3 participants