Closed kavimuru closed 1 year ago
Triggered auto assignment to @Christinadobrzyn (AutoAssignerTriage
), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
Triggered auto assignment to @youssef-lr (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
@Christinadobrzyn 👋 just chiming in to say that you should feel free to update the GH to include desktop app
in the list of affected platforms next time! I added it this time, but that's a valuable addition when triaging :)
@youssef-lr - just a friendly bump here. Is this something you have the bandwidth to work on or should it go back to the pool?
Heads up Youssef is OOO today and posted internally in #social. I'm sure he'll get to it monday!
On Fri, Oct 14, 2022 at 2:45 PM Sheena Trepanier @.***> wrote:
@youssef-lr https://github.com/youssef-lr - just a friendly bump here. Is this something you have the bandwidth to work on or should it go back to the pool?
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11735#issuecomment-1279345224, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADEYK4G6QMAH5USOJG3C353WDGS53ANCNFSM6AAAAAARCXGAH4 . You are receiving this because you were mentioned.Message ID: @.***>
-- Michael Haxhiu
@youssef-lr Eep! 4 days overdue now. Issues have feelings too...
I agree this is an issue, adding External
.
Triggered auto assignment to @muttmuure (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External
)
Triggered auto assignment to @chiragsalian (External
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Price has been doubled to $500
Not overdue, waiting on proposals
Btw, we should double the price at this point.
Price has been doubled to [$1000]! Posting to original thread
- style={this.props.containerStyles}
+ style={[this.props.containerStyles, styles.dInline]}
- style={this.props.containerStyles}
+ style={[this.props.containerStyles, styles.dInline]}
@b1tjoy did some text in your proposal draft get deleted/removed? Seems a bunch of header fields that you didn't populate? Even if it's a simple 1 liner for each of those headers, I think it's a good idea to add them for context so the C+ can fully grasp the solution
Proposal
The root cause of this bug is the behaviour of nested containers when using the css style display: "inline-flex"
. I have isolated the behaviour in this codepen
This style is found on the containers added by tooltip
by default, these wrap the link which causes the issue.
To resolve this we can instead apply the style display: "inline"
specifically to the tooltip container for links in comments. This doesn't affect the layout as flex is redundant in this case, the only child of these containers being a single link.
- <Tooltip text={Str.isValidEmail(props.displayName) ? '' : props.href}>
+ <Tooltip containerStyles={styles.dInline} text={Str.isValidEmail(props.displayName) ? '' : props.href}>
This resolves the issue on large screens, however there are two more small issues. Firstly this doesn't work on Safari because in the defaultTextStyle applied to the link we have neglected to add WebkitUserSelect
, so we can add it here:
userSelectText: {
userSelect: 'text',
+ WebkitUserSelect: 'text',
},
Secondly, userSelect
is disabled on small screens rendering the link unselectable. We can resolve this by removing the logic that disables it on all small screens, leaving it disabled only on touchscreen devices.
- const defaultTextStyle = canUseTouchScreen() || props.isSmallScreenWidth ? {} : styles.userSelectText;
+ const defaultTextStyle = canUseTouchScreen() ? {} : styles.userSelectText;
@michaelhaxhiu Thanks for reminding me, it's an incomplete proposal posted in a rush, you can just ignore it, @Ollyws will take care of this issue.
Thanks @b1tjoy sorry about that, coincidental timing.
@thesahindia @wildan-m, i'm wondering if the solution proposed here could be more generalized to tackle this issue too. Thoughts?
@chiragsalian, I'm not sure, I think this one is a different case, the link is still not selectable after this solution is applied.
Yup, it's a different issue.
Cool, thanks for double-checking.
@Ollyws's proposal is good, resolves the current issue, URL's are highlighted when triple-clicking a DM.
Secondly, userSelect is disabled on small screens rendering the link unselectable
I think that's the expected behavior
However, it introduces a new bug when copying a fully selected message to a clipboard. If you triple-click a DM and then press Cmd+C, it seems to append the concierge welcome message to the selected message
https://user-images.githubusercontent.com/9059945/202253450-91d2b955-04b1-4a9e-9276-3c80a18254b6.mov
@eVoloshchak Interesting, that bug only started to occur when I pulled the latest changes. It doesn't seem to occur if you apply the changes from @wildan-m's latest PR
If you triple-click a DM and then press Cmd+C, it seems to append the concierge welcome message to the selected message
That's an already known bug which we are handing at https://github.com/Expensify/App/pull/12730
@Ollyws, @thesahindia, cool cool, thank you for pointing to that PR
In that case I think we should proceed with @Ollyws's proposal (minus the part with disabled userSelect
on small screens)
🎀👀🎀 C+ reviewed! cc: @chiragsalian
Sounds good, @Ollyws proposal LGTM as well. Feel free to create the PR @Ollyws.
📣 @Ollyws You have been assigned to this job by @chiragsalian! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
All yours @eVoloshchak. Let's see if we can get this merged within three days!
@chiragsalian is this complete?
@muttmuure, this was deployed to staging a couple of hours ago
Yes, its on staging right now but not on production just yet.
The job posting had expired (sorry!) but I've created a new one and it's here.
https://www.upwork.com/jobs/~01d70533c5adde99c5
@Ollyws @eVoloshchak please apply and we can get you both paid.
@muttmuure Applied, thanks!
Everyone has been paid, bug is fixed and there are no regressions. Great work everyone!
@muttmuure I think you may have forgot the bonus, we did get it merged within 24 hours.
@muttmuure I think you may have forgot the bonus, we did get it merged within 24 hours.
Same By the way, what is the process with bonuses? Who should track if C and C+ are eligible for bonuses? Contributor, Contributor+, or CME?
oops! Fixing that
It's up to me on the BugZero team to check. I just missed it this time.
It's up to me on the BugZero team to check. I just missed it this time.
Got it, thanks!
50% Bonus applied - again, thanks for the speedy work
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
URL also should get highlighted
Actual Result:
URL is not getting highlighted
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.12-3 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:
https://user-images.githubusercontent.com/43996225/195210767-104b58d5-f4b9-4dd2-9fd4-2219d4070f90.mp4
https://user-images.githubusercontent.com/43996225/195210785-c885d964-1966-4018-a860-56c4e9976af6.mp4
Expensify/Expensify Issue URL: Issue reported by: @michaelhaxhiu Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665524227683689
View all open jobs on GitHub