[refactor] [native] [ENG-530] Remove positioning prop from InlineSidebar component
AbandonedPublic
Actions

Authored by • benschac on May 9 2022, 10:33 AM.

Details

Reviewers

• jacek
tomek
• derek
atul

Summary

no longer use this to style inline-sidebar anymore,

Test Plan

project should run and work like it did before, just removing a prop.

Diff Detail

Repository

rCOMM Comm

Branch

inline-sidebar-web-and-native-ENG-530

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• benschac created this revision.May 9 2022, 10:33 AM

Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptMay 9 2022, 10:33 AM

• benschac added a reviewer: • derek.May 9 2022, 10:33 AM

• benschac added a parent revision: D3915: [web,native,lib] [refactor] [ENG-530] rename hook to useInlineEngagement from useInlineText.

Harbormaster completed remote builds in B8937: Diff 12447.May 9 2022, 10:38 AM

• benschac requested review of this revision.May 9 2022, 10:38 AM

It looks like the position[ing] prop isn't used in InlineSidebar at all. Looks like it should be removed altogether?

This revision now requires changes to proceed.May 9 2022, 10:47 AM

• benschac added a child revision: D3972: [native] [reactor] [ENG-530] fix hot reload export issue in chat message.May 9 2022, 12:33 PM

In D3971#111310, @atul wrote:

It looks like the position[ing] prop isn't used in InlineSidebar at all. Looks like it should be removed altogether?

Yeah, I thought I would use it positioning the reply bubble but I guess I'm probably not going to at this point.

remove positioning

• benschac retitled this revision from [refactor] [native] [ENG-530] update positioning --> position props, consistent with web to [refactor] [native] [ENG-530] remove positioning prop.May 9 2022, 1:22 PM

• benschac edited the summary of this revision. (Show Details)

• benschac edited the test plan for this revision. (Show Details)

Nice, thanks for removing the prop altogether!

[refactor] [native] [ENG-530] remove positioning prop

Minor nit, but might be able to title the diff a little better. I think something like: "[native] Remove positioning prop from InlineSidebar component" would be clearer. I never look up a Linear issue by ID, so that just adds clutter imo. However, I will definitely always click a link to a Linear issue if there's one provided in the description.

This revision is now accepted and ready to land.May 10 2022, 6:57 AM

Strong agree with everything @atul said regarding diff naming – @benschac's diff titles are very complex to parse for me, and I think the Linear issue number is just adding clutter and should be replaced with a link in the diff description

Harbormaster completed remote builds in B8940: Diff 12450.May 10 2022, 8:14 AM

• benschac retitled this revision from [refactor] [native] [ENG-530] remove positioning prop to [refactor] [native] [ENG-530] Remove positioning prop from InlineSidebar component.May 10 2022, 8:40 AM

In D3971#111902, @ashoat wrote:

Strong agree with everything @atul said regarding diff naming – @benschac's diff titles are very complex to parse for me, and I think the Linear issue number is just adding clutter and should be replaced with a link in the diff description

The point of putting the Linear task ID in the title is to make it easy for to see at a high level which diff belongs to each task. I find it helpful when I'm looking through my own diffs to make sense of which diff I'm clicking into.

I updated the name to @atul's suggestion.

• benschac mentioned this in D4001: [native] [feat] [ENG-530] add new styles to inline sidebar component.May 24 2022, 9:42 AM

(Copied from D3893)

@jacek I think we can abandon these changes since you've started a separate stack to deal with InlineSidebar. Let me know if I'm mistaken and I can re-open this if it tracks some unique work.

This revision now requires review to proceed.Aug 15 2022, 9:50 AM

atul abandoned this revision.Aug 15 2022, 9:50 AM

atul foisted this revision upon • benschac.

atul edited reviewers, added: atul; removed: • benschac.