Skip to content

Conversation

@AhyoungRyu
Copy link
Contributor

@AhyoungRyu AhyoungRyu commented Oct 25, 2024

Aim to fix https://sendbird.atlassian.net/browse/SBISSUE-17616

Description

Fixed an issue where the dir attribute was not being properly applied to message containers. Instead of using DOM manipulation through useMessageLayoutDirection hook, implemented a more React-friendly solution by directly setting the dir attribute on the message container component.

Changes

  • Removed useMessageLayoutDirection hook
  • Updated MessageList component to directly handle text direction through the dir attribute
  • Simplified the implementation while maintaining the same functionality
  • Ensures proper text direction propagation to child elements

Testing

Please test the following scenarios:

  • Verify text direction changes properly when switching between LTR and RTL by changing the value of forceLeftToRightMessageLayout / hTMLTextDirection
  • Check message alignment in both directions
  • Confirm direction changes are properly propagated to child elements

Note

This change simplifies our approach to handling text direction while maintaining full compatibility with our RTL support system.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If unsure, ask the members.
This is a reminder of what we look for before merging your code.

  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • Public components / utils / props are appropriately exported
  • I have added necessary documentation (if appropriate)

External Contributions

This project is not yet set up to accept pull requests from external contributors.

If you have a pull request that you believe should be accepted, please contact
the Developer Relations team [email protected] with details
and we'll evaluate if we can set up a CLA to allow for the contribution.

@netlify
Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for sendbird-uikit-react ready!

Name Link
🔨 Latest commit 68cec91
🔍 Latest deploy log https://app.netlify.com/sites/sendbird-uikit-react/deploys/67208140339504000879d51f
😎 Deploy Preview https://deploy-preview-1242--sendbird-uikit-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bang9
Copy link
Contributor

bang9 commented Oct 25, 2024

I have a question. Is the reason we need to manipulate the DOM directly because there isn't a top-level component to specify the direction? If that's not the case, wouldn't it be better to cover it using inline styles or classnames to align with the React lifecycle?

@AhyoungRyu
Copy link
Contributor Author

@bang9 Thanks for the question. The direct DOM manipulation using dir attribute here is intentional and necessary due to following reasons.

  1. CSS Specificity and RTL Support
    We're using the dir attribute specifically because it works with our PostCSS RTL plugin configuration (see: postcssRtlOptions.mjs). The plugin generates RTL styles based on this attribute.

  2. Technical Requirements

    • Using inline styles wouldn't achieve the same effect as the dir attribute, as dir is specifically designed for controlling text direction and affects how nested elements behave
    • While we could potentially use classNames, the dir attribute provides better semantic meaning and works more reliably with bidirectional text
  3. Current Architecture
    The messages container doesn't have a single top-level component where we could set this property through React props, and restructuring this would require changes to the component hierarchy unfortunately 🥲

This approach(while not typically React-like..) is the most effective solution for our specific use case of handling bidirectional text support in the message layout with the RTL plugin we're currently using.

Not sure this can be a proper answer on your question but let me know if you have any other thoughts :)

@bang9
Copy link
Contributor

bang9 commented Oct 25, 2024

Ah, so dir is provided as an element attribute, not a style property.

I understand that there isn’t a single DOM element to set a global dir for the entire app.

However, I think it might be a bit different in the case of the message container. As far as I know, setting dir on an element propagates to all child elements, so if we set dir on the message container itself, the messages within (as child elements) should automatically adjust their alignment. How about testing this approach?

export const MessageList = () => {
  // ...
  return (
    <div dir={forceLeftToRightMessageLayout ? 'ltr' : ''} className={`sendbird-conversation__messages ${className}`}>
      <InfiniteList
        // ...
      />
    </div>
  )
}

Copy link
Collaborator

@chrisallo chrisallo left a comment

Choose a reason for hiding this comment

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

lgtm!

@AhyoungRyu
Copy link
Contributor Author

@bang9 Thanks for the suggestion! You're right. I've tested your approach and it's able to solve the problem without regressions. If useMessageLayoutDirection needs to handle more complex logic in the future, your suggested approach would indeed be easier to maintain. I've updated the implementation - please check 68cec91

@AhyoungRyu AhyoungRyu merged commit 197e037 into main Oct 29, 2024
5 checks passed
@AhyoungRyu AhyoungRyu deleted the fix/CLNP-5549 branch October 29, 2024 07:05
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.

4 participants