add react-icons dependency so we can use Ionicons icon pack
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Visually looks great!
...but, we should probably use the same icon pack as we are on native for consistency.
| web/chat/chat-thread-list-see-more-sidebars.react.js | ||
|---|---|---|
| 42 ↗ | (On Diff #16569) | I think we typically want to use icons from the SWMansion icon pack... but it looks like their ellipsis icons have holes in them?  The icons we use on native do not have holes in them, and I think matching native is more of a priority than sticking to SWMansion icons. It looks like the icons we're using on native are from the Ionicons pack, which aren't included in web so you're using react-feather instead. In this case I think it's okay since the icon is very simple and web and native look similar side-by-side:  However, to keep things consistent across web and native it'd be good to use Ionicons on web as well. Based on a quick Google search it looks like react-icons (https://github.com/react-icons/react-icons) might be the easiest/best-supported way to include Ionicons on web. Could you try including Ionicons via react-icons and see if we can use their ellipsis icon (I believe named ios-more) instead? I'd personally create a separate diff to add the react-icons dependency so the changes to package.json and yarn.lock don't clutter up this existing diff. CC @ashoat since I'm proposing including react-icons dependency (https://github.com/react-icons/react-icons) |
| web/chat/chat-thread-list.css | ||
| 175 ↗ | (On Diff #16569) | Setting padding-left to 20px looks like it leads to better alignment: Before:  After:  |
Comment Actions
Reasonable, although an alternate option would've been to move native to use whatever @ginsu was initially using for web here (which would potentially avoid adding a new dependency to web)
Note – if we move forward with react-icons, it's critical that we make sure it doesn't bloat the size of our JS bundle. We should compare JS bundle size before/after the relevant import from react-icons is added
Comment Actions
@ashoat used this import cost tool to calculate the bundle size of react-icons and got a size of 2.2kb (1kb when gZipped). I think this is pretty reasonable in terms of size, but your idea of switching the icons from native to web is also solid. Let me know what you think, and I'll go in the direction you think is best
{F171155}