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

Change many border widths from 1 to hairlineWidth #4294

Merged
merged 30 commits into from
May 31, 2024
Merged

Conversation

haileyok
Copy link
Member

@haileyok haileyok commented May 31, 2024

Why

Note: We are changing #2090 to round to 0.5 instead, since hairline is 0.5 pixels.

Hairline width looks a lot better in many places.

I also adjusted a few more things while going through the codebase, see individual comments for details.

Test Plan

Honestly, I think I've hit everything visually by clicking through the app on a simulator, though I'm not certain. Will need to continue using it.

Copy link

render bot commented May 31, 2024

<ViewHeader
title={_(msg`Notifications`)}
canGoBack={false}
showBorder={true}
Copy link
Member Author

Choose a reason for hiding this comment

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

had to add border here, since previously it was on the top of the post item.

@@ -27,6 +33,7 @@ let FeedSlice = ({slice}: {slice: FeedPostSlice}): React.ReactNode => {
moderation={slice.items[0].moderation}
isThreadParent={isThreadParentAt(slice.items, 0)}
isThreadChild={isThreadChildAt(slice.items, 0)}
hideTopBorder={hideTopBorder}
Copy link
Member Author

Choose a reason for hiding this comment

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

In feeds, we use FeedSlice instead of FeedItem directly. So, we need to tell the first item in the feed slice to not display the border if it is also the first item in renderItem.

@@ -75,6 +82,7 @@ let FeedSlice = ({slice}: {slice: FeedPostSlice}): React.ReactNode => {
isThreadLastChild={
isThreadChildAt(slice.items, i) && slice.items.length === i + 1
}
hideTopBorder={hideTopBorder && i === 0}
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

Comment on lines +184 to +185
initialNumToRender={initialNumToRender}
windowSize={11}
Copy link
Member Author

Choose a reason for hiding this comment

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

We were missing some FlatList perf props here. Removed the above comment after adding this, since I wasn't even getting the warning previously anyway.

Copy link

github-actions bot commented May 31, 2024

Old size New size Diff
7.23 MB 7.24 MB 837 B (0.01%)

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

need to fix

  • search/hashtags (between posts)
  • feeds page
  • lists page header
  • individual list page header
  • mod list page header

i'm not sure i dig them on account switcher but this may be a spacing issue. not sure.

overall looks great to me

Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

lets gooooo

@gaearon gaearon merged commit 89c9fd3 into main May 31, 2024
6 checks passed
@gaearon gaearon deleted the hailey/hairline-width branch May 31, 2024 04:32
estrattonbailey added a commit that referenced this pull request May 31, 2024
* origin/main: (51 commits)
  [🐴] Option to share via chat in post dropdown (#4231)
  [🐴] Record message (#4230)
  [🐴] send record via link in text (Record DMs - base PR) (#4227)
  Use new icons on notifications screen (#4299)
  Composer - fix modals, and other tweaks (#4298)
  Shadows (#4265)
  Change many border widths from `1` to `hairlineWidth` (#4294)
  Add follow button to feed item avatar (#3560)
  Disable non-deterministic flaky test (#4295)
  Don't log downsample warning when unnecessary (#4291)
  [Statsig] Sample noisy events (#4288)
  Bump FontAwesome (#4285)
  Fix scrolling for labeler profiles (#4286)
  Reduce Threadgate button size (#4287)
  put dropdown in fullscreenoverlay on iOS (#4284)
  play haptics before closing modal (#4283)
  match loadmore position to fab (#4280)
  Composer - Use sheet presentation on iOS (#4278)
  don't maintain position whenever there are no parents (#4277)
  Fix native translations on iOS 17.5.1 (#4282)
  ...
estrattonbailey added a commit that referenced this pull request May 31, 2024
* origin/main: (37 commits)
  fix accessibility label in notifications (#4305)
  [🐴] add emoji multiplier prop to RichText and bump it up for DMs (#4229)
  Tweak avi follow button styles (#4304)
  [🐴] Embed backwards compat (#4302)
  [🐴] Add labels to chats (#4293)
  [🐴] Option to share via chat in post dropdown (#4231)
  [🐴] Record message (#4230)
  [🐴] send record via link in text (Record DMs - base PR) (#4227)
  Use new icons on notifications screen (#4299)
  Composer - fix modals, and other tweaks (#4298)
  Shadows (#4265)
  Change many border widths from `1` to `hairlineWidth` (#4294)
  Add follow button to feed item avatar (#3560)
  Disable non-deterministic flaky test (#4295)
  Don't log downsample warning when unnecessary (#4291)
  [Statsig] Sample noisy events (#4288)
  Bump FontAwesome (#4285)
  Fix scrolling for labeler profiles (#4286)
  Reduce Threadgate button size (#4287)
  put dropdown in fullscreenoverlay on iOS (#4284)
  ...
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.

None yet

2 participants