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

Fix setting border radius on web in singleline mode #617

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

Skalakid
Copy link
Collaborator

Details

This PR fixes adding border radius style to the markdowns in the single line input mode.

Related Issues

GH_LINK

Manual Tests

  1. Add, for example
      mentionUser: {
        borderRadius: 5,
      },

to the markdownStyles
2. Change the MarkdownTextInput component to the single line mode by deactivating the multiline flag
3. Verify if the border radius style was applied

Linked PRs

BartoszGrajdek
BartoszGrajdek previously approved these changes Feb 10, 2025
Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@BartoszGrajdek
Copy link
Collaborator

Just to confirm - if we're not supporting border radius on native yet there's no need to add it here right? @tomekzaw

@tomekzaw
Copy link
Collaborator

We don't need to add it to native right now, we can do that later.

@Skalakid Skalakid marked this pull request as ready for review February 13, 2025 09:27
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure how we should handle these types. It can mislead developers into thinking that we support borderRadius on native devices. Maybe we should make a separate type for MarkdownStyle on web that will be a union of this interface and a new type solely for the web?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had similar thoughts about the difference between which styles are supported on web vs mobile.
What complicates things more is that for web we just reassign all the styles from style argument, so in runtime (from JS perspective) you can apply anything and it will work most of the time, even though Types prevent.

For us in E/App in Search we could apply both borderRadius and padding to mention-user and it looked fine, but TS was throwing an error since types did not support this.
However I don't have any concrete ideas to improve this ATM.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past, we discussed changing web styles so that only the ones we support would be accepted. This shouldn't be too hard to implement; I'm just not sure if that's exactly what we're aiming for.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that having borderRadius in markdown style also on native is a problem. We can just leave it as a no-op for now.

Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Skalakid Skalakid merged commit 0e8cf49 into main Feb 17, 2025
5 checks passed
@Skalakid Skalakid deleted the @Skalakid/fix-setting-background-border-radius branch February 17, 2025 09:17
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.

4 participants