knocklabs / javascript

Official JavaScript packages for interacting with Knock
https://knock.app/
MIT License
14 stars 3 forks source link

Unexpected CSS shift with migration #61

Closed jtferns closed 7 months ago

jtferns commented 7 months ago

When trying to migrate from @knocklabs/react-notification-feed v0.8.11 to @knocklabs/react v0.1.6, following the migration guide, we ran into an odd CSS / styling shift.

1️⃣ Did anything change in how the default theming applied for the Notification feed component / internals? 2️⃣ What would be required to restore the in-feed notif relative timestamp text (e.g., 10 months ago)?

connorlindsey commented 7 months ago

For styling, could you confirm two things from the migration guide were completed?

  1. The CSS import should be updated to import styles from the new package import "@knocklabs/react/dist/index.css";
  2. If you're not using the headless mode, you should wrap your feed in the <NotificationFeedContainer />

For the timestamp, we recently updated how we do relative timestamp formatting to use the built in Intl module with the goal of reducing the package bundle size. This should work the exact same but looks like there might be an issue. What browser/OS are you seeing this in and do you see the issue in other browsers? Also, are you setting the locale when create the knock providers?

jtferns commented 7 months ago

1️⃣ Confirmed, the CSS import was updated as part of the migration

- import '@knocklabs/react-notification-feed/dist/index.css';
+ import '@knocklabs/react/dist/index.css';

2️⃣ Ah, I'm not sure about headless mode, but our feed usage is almost identical to the example in the docs (with the only differences being the token/user information wiring).

For the timestamp, I was testing the dependency upgrade on Chrome 121/Mac 13.4; did not get a chance to confirm on Safari/Firefox, but can test those if needed. We are only passing apiKey, userId, and userToken to the Knock provider; should we also supply a locale there?

connorlindsey commented 7 months ago

@jtferns The feed container component ensures that our components have the right CSS scoping, so I'd make sure to add that: https://docs.knock.app/in-app-ui/react/migrating-from-react-notification-feed#rootless-removed-and-notificationfeedcontainer-added

jtferns commented 7 months ago

🙇 Thanks @connorlindsey ! I'm not sure how I missed that in migration guide, but replacing the fragment with the feed container component fixed 1️⃣ the theming issues and 2️⃣ timestamp formatting:

Screenshot 2024-02-22 at 2 03 42 PM

connorlindsey commented 7 months ago

@jtferns Awesome, glad that worked! Will see if we can make the migration guide clearer.

debsouryadatta commented 4 months ago

Thanks, I was wondering out why the styles were not visible after installation. Adding the lin "import '@knocklabs/react/dist/index.css';" did the work for me.