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

Ability to override predefined style object #48

Closed
Multiply opened this issue Jan 31, 2020 · 8 comments
Closed

Ability to override predefined style object #48

Multiply opened this issue Jan 31, 2020 · 8 comments

Comments

@Multiply
Copy link

Multiply commented Jan 31, 2020

I'd love to be able to override or entirely disable the current style object.
Especially the verticalAlign property is messing up my layout.

I've made a local change to fix the issue on my end, but it's very crude (I am just removing all properties, and setting them through a className instead)

@enzoferey
Copy link
Collaborator

Hi @Multiply, could you please post a code snippet so I can better understand your need ?

@Multiply
Copy link
Author

Multiply commented Jan 31, 2020

The problem is the generated spans have a hardcoded style tag on them.
Due to the verticalAlign being negative, and me having an auto-scroll-to-bottom plugin running, they often clash, when a text with an emoji is rendered. (Edit: some clarification, it doesn't register that it's no longer scrolled to the bottom, so it stops keeping it scrolled down)

Ideally verticalAlign should be set by the parent element, or at least through CSS so it can be customized more easy.

So <Emoji text="Hi <3 !" /> will generate something like <span>Hi <span style="...">❤️</span>!</span>

@enzoferey
Copy link
Collaborator

I see. What I don't understand is how the hardcoded verticalAlign (which is used to align vertically emojis with surrounding text) causes clashes with your auto-scroll-to-bottom plugin. You should be able to target the wrapping span tag (not the one with verticalAlign, but the one that contains the text) and target that node's rect.y as a reference to where to scroll.

@Multiply
Copy link
Author

If you want to align the emoji with the rest of the text, wouldn't it be more ideal to use verticalAlign: middle on the parent?

The Emoji component is used on multiple lines in a single parent div. That div is then automatically scrolled, but the browser doesn't properly detect a change in scroll.

@enzoferey
Copy link
Collaborator

enzoferey commented Jan 31, 2020

For the record, these styles are declared at https://github.com/tommoor/react-emoji-render/blob/master/src/renderer.js#L20.

I didn't write them so I don't know the motivation why verticalAlign was used like this. I do agree that vertical-align: middle in the parent would center vertically (which doesn't produce the same output though). cc @tommoor

In the meantime, I would propose to spread options.props as we do for the case where we should render a img. This way we could write:

<Emoji text="Hi <3 !" options={{ props: { style: { ... } } }} />

Not the best API for sure, but at least you wouldn't need to use !important. Also, it makes sense to have uniform spreading between the nodes the toArray function returns.

Finally, I still don't see why your scrolling script wouldn't work as this vertical align thing doesn't change the height of the bounding rect, but I guess there is something I'm not aware of 😅

@Multiply
Copy link
Author

Finally, I still don't see why your scrolling script wouldn't work as this vertical align thing doesn't change the height of the bounding rect, but I guess there is something I'm not aware of

It beats me as well. Using no styles, it works as expected. I honestly didn't debug much further into it.
Thanks for the options property mention. I didn't expect it to be able to override it. I'll try that.

@enzoferey
Copy link
Collaborator

enzoferey commented Jan 31, 2020

The options property is currently not being spread when not using an image based emoji, so it wouldn’t work at the moment.

Feel free to open a PR 👍🏻

@Multiply
Copy link
Author

Closing the issue as I solved it by just using the Twemoji variant.

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

No branches or pull requests

2 participants