Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.54k stars 2.89k forks source link

[$500] Private Note - Left line/border of quoted text is not visible when hovering over it in Notes #28858

Closed kbecciv closed 9 months ago

kbecciv commented 1 year ago

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:

  1. Go to staging.
  2. Access any chat.
  3. Click on the header and navigate to "Private notes."
  4. Go to My note > Notes
  5. Enter "> test" and save it.
  6. Observe that the left line isn't visible on hovering.

Expected Result:

The left line should remain visible when hovering

Actual Result:

The left line is hidden

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.77.5 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/d393f7d5-e16d-49b9-a80a-3b44ccb60de7

https://github.com/Expensify/App/assets/93399543/2b266b24-8eeb-4b24-add8-b7f53beb9c31

https://github.com/Expensify/App/assets/93399543/5ecc3c78-6144-47b5-a758-78f6f6ebf7a6

Expensify/Expensify Issue URL: Issue reported by: @aman-atg Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696349601547269

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a4212fb13e89f5bb
  • Upwork Job ID: 1709663891418292224
  • Last Price Increase: 2023-10-25
  • Automatic offers:
    • hoangzinh | Reviewer | 27460002
    • ZhenjaHorbach | Contributor | 27460004
    • aman-atg | Reporter | 27460007
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @peterdbarkerUK (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01a4212fb13e89f5bb

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External)

ZhenjaHorbach commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Blockquote has the same color as the selected color

What is the root cause of that problem?

Blockquote has the same color as the private note background color while hovering

In the styles we use for blockquote borderLeftColor: theme.border which matches the color of the background after hovering

Therefore, after hovering over an element, the blockquote merges with the background

What changes do you think we should make in order to solve the problem?

Due to the latest updates, there was a decision to update the background color for menu items

We need to update getButtonBackgroundColorStyle

function getButtonBackgroundColorStyle(buttonState: ButtonStateName = CONST.BUTTON_STATES.DEFAULT, isMenuItem = false): ViewStyle {
    switch (buttonState) {
        case CONST.BUTTON_STATES.PRESSED:
            return isMenuItem ? {backgroundColor: themeColors.border} : {backgroundColor: themeColors.buttonPressedBG}; // or just {backgroundColor: themeColors.border}
        case CONST.BUTTON_STATES.ACTIVE:
            return isMenuItem ? {backgroundColor: themeColors.highlightBG} : {backgroundColor: themeColors.buttonHoveredBG};
        case CONST.BUTTON_STATES.DISABLED:
        case CONST.BUTTON_STATES.DEFAULT:
        default:
            return {};
    }
}

https://github.com/Expensify/App/blob/55f86ce3c9f4bf9e149c9bd258576f0f4dcfb977/src/styles/StyleUtils.ts#L521-L532

What alternative solutions did you explore? (Optional)

To display text for MenuItem we use RenderHTMLSource

Which use one global set of styles What has a positive effect on performance But we can not pass custom styles

https://github.com/Expensify/App/blob/4078503d8e2468a3f0c54fc37af0f888ed2abf41/src/components/RenderHTML.js#L15-L23

If we decide to change the color on hover we will have to refactor this component

And use RenderHTML

function RenderHTMLComponent(props) {
    const {windowWidth} = useWindowDimensions();
    if (props.customTagStyles) {
        return (
            <RenderHTML
                contentWidth={windowWidth * 0.8}
                baseStyle={styles.webViewStyles.baseFontStyle}
                tagsStyles={props.customTagStyles}
                source={{html: props.html}}
            />
        );
    }
    return (
        <RenderHTMLSource
            contentWidth={windowWidth * 0.8}
            source={{html: props.html}}
        />
    );
}

https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/components/RenderHTML.js

And after that, we will be able to pass custom styles

For example

                                                <MenuItemRenderHTMLTitle
                                                    title={getProcessedTitle}
                                                    customTagStyles={{
                                                        blockquote: {
                                                            ...styles.webViewStyles.tagStyles.blockquote,
                                                            borderLeftColor: isHovered ? 'red' : 'blue',
                                                        },
                                                    }}
                                                />

https://github.com/Expensify/App/blob/f878fbd0f06ad7e6eb033a354b4140dfbc5fd3db/src/components/MenuItem.js#L254-L256

Or we can pass these styles as props to MenuItem

dannysanchez559 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The left border on quoted text is not visible when hovering in Notes.

What is the root cause of that problem?

This can be caused by a CSS styling issue. I believe it may be the hover attribute

What changes do you think we should make in order to solve the problem?

make sure the border-left attribute in hover is working properly and is visible in the hover state

What alternative solutions did you explore? (Optional)

N/A

Contributor details Your Expensify account email: dan.sanchez823+expensifytesting@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/danielsanchez112?viewMode=1

melvin-bot[bot] commented 1 year ago

📣 @dannysanchez559! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
Kunkka0822 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The left border on quoted text is not visible when hovering in Notes.

What is the root cause of that problem?

The reason of this issue is that the background color of "Private notes" and the color of left bar is same. Please take a look at below two screenshots. This image shows the color of left bar when not hovering. image

And this image shows the background color of "Notes" image As you can see, both are using same color value. RGB(26, 61, 50)

What changes do you think we should make in order to solve the problem?

We should change the color of left bar or background color of the "Notes". Like this below image. image This is a simple issue to fix easily.

What alternative solutions did you explore? (Optional)

N/A

Contributor details Your Expensify account email: kotenkoatem2019@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/maksyml22

melvin-bot[bot] commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

hoangzinh commented 1 year ago

@ZhenjaHorbach, Thanks for your proposal. Could you add more details to your RCA? and do you mean "hover" instead of "selected color"?

hoangzinh commented 1 year ago

@dannysanchez559 Thanks for your proposal, but your proposal is not clear enough to me. I couldn't get the root cause and also the idea to fix the issue. Do you mind to check your proposal again? Thanks

ZhenjaHorbach commented 1 year ago

@hoangzinh It was just an idea ) What to set a different color for blockquote or private note during hover ) But I guess that doesn't make sense Changing default color for blockquote looks simpler and more concise

plus update proposal ) https://github.com/Expensify/App/issues/28858#issuecomment-1747586975

hoangzinh commented 1 year ago

@Kunkka0822 Thanks for your proposal. You have the correct RCA, but next time, it's better if you can link to the current code into your RCA (so we can have broader information based on the codebase). Unforturnely, Your approach is almost the same as @ZhenjaHorbach's approach. And I prefer the idea of @ZhenjaHorbach with changing to borderLeftColor: theme.borderLighter.

Anyway, keep up your great work in other issues.

hoangzinh commented 1 year ago

It was just an idea )

@ZhenjaHorbach yeah, but RCA needs to clear as much as possible to we can ensure we're fixing the root cause. Btw, could you help to take screenshots of before and after change to borderLeftColor: theme.borderLighter of blockquote in Note and also in chat report message? I think we need design approve in this case, so we need screenshots. Thanks

ZhenjaHorbach commented 1 year ago

@hoangzinh Done )

Before borderLeftColor: theme.border

https://github.com/Expensify/App/assets/68128028/392c1fd4-606d-46eb-b995-c7445ee36af4

After borderLeftColor: theme.borderLighter

https://github.com/Expensify/App/assets/68128028/11435f97-3566-4099-910b-5037998dfc22

hoangzinh commented 1 year ago

@shawnborton @peterdbarkerUK , we need your help to review if the new border color of the blockquote looks good to you.

In this GH, We identified that the current blockquote's border and hover background of menu item have the same color theme.border (#1A3D32). It makes the blockquote hidden when we hover over the menu item.

@ZhenjaHorbach suggested in his proposal https://github.com/Expensify/App/issues/28858#issuecomment-1747586975 that we should update blockquote's borderline to theme.borderLighter (#184E3D). Could you help to review if the blockquote still looks good? Thanks

Below are screenshots before & after of blockquote in Private notes and Chat Message

Default state

Before After
Screenshot 2023-10-05 at 22 38 34 Screenshot 2023-10-05 at 22 41 06

Hover state

Before After
Screenshot 2023-10-05 at 22 39 02 Screenshot 2023-10-05 at 22 41 09

Default state

Before After
Screenshot 2023-10-05 at 22 42 44 Screenshot 2023-10-05 at 22 43 01

Hover state

Before After
Screenshot 2023-10-05 at 22 42 48 Screenshot 2023-10-05 at 22 43 05

And recording video of @ZhenjaHorbach here https://github.com/Expensify/App/issues/28858#issuecomment-1748937582

shawnborton commented 1 year ago

I think we should only change the border color on :hover for the notes push rows for private notes. We should keep it the same elsewhere though.

ZhenjaHorbach commented 1 year ago

I think we can do it But there is a question of performance here

To display text for MenuItem we use RenderHTMLSource

Which use one global set of styles What has a positive effect on performance But we can not pass custom styles

https://github.com/Expensify/App/blob/4078503d8e2468a3f0c54fc37af0f888ed2abf41/src/components/RenderHTML.js#L15-L23

If we decide to change the color on hover we will have to refactor this component

And use RenderHTML

function RenderHTMLComponent(props) {
    const {windowWidth} = useWindowDimensions();
    if (props.customTagStyles) {
        return (
            <RenderHTML
                contentWidth={windowWidth * 0.8}
                baseStyle={styles.webViewStyles.baseFontStyle}
                tagsStyles={props.customTagStyles}
                source={{html: props.html}}
            />
        );
    }
    return (
        <RenderHTMLSource
            contentWidth={windowWidth * 0.8}
            source={{html: props.html}}
        />
    );
}

Which will have to pass a custom set of styles

                                                <MenuItemRenderHTMLTitle
                                                    title={getProcessedTitle}
                                                    customTagStyles={{
                                                        blockquote: {
                                                            ...styles.webViewStyles.tagStyles.blockquote,
                                                            borderLeftColor: isHovered ? 'red' : 'blue',
                                                        },
                                                    }}
                                                />

But in this solution we have problem

https://stackoverflow.com/questions/68966120/react-native-render-html-you-seem-to-update-the-x-prop-of-the-y-component-in-s

I tried a little to get rid of this warning But I haven't succeeded yet

Since our parent element changes very often, especially when we hover over the element and change the state

In case the style set is static, I think we can use this

But if we are talking about color changes on hover, it looks very complicated and unjustifiably

@hoangzinh

What do you think ?

ZhenjaHorbach commented 1 year ago

@shawnborton

What do you think about if we change the default color only on private notes? and we will not depend on whether the element is hovered or not?

shawnborton commented 1 year ago

I think we should use our standard border color where ever we can, and only use this exception on hover.

That being said, I'm actually surprised we're showing the border here:

CleanShot 2023-10-05 at 14 02 29@2x

Why aren't we just rendering this as plain text? That seems odd to me that we'd show that kind of styling in a push-to-page input. cc @Expensify/design for thoughts, as well as @techievivek @JmillsExpensify @trjExpensify

hoangzinh commented 1 year ago

There are 2 reasons that I think we can use a lighter color of our standard border for blockquote:

  1. This bug can be reproducible in the Task details report as well. So I think about a global solution that can fix for other pages if possible.
  2. We also apply themeColors.borderLighter as the border color of the checkbox as well (code link)

https://github.com/Expensify/App/assets/9639873/43a93da1-42e5-4b4c-af83-5a55725234fc

Why aren't we just rendering this as plain text? That seems odd to me that we'd show that kind of styling in a push-to-page input.

I have the same thought. But I realize we're supporting markdown on various pages I.e: Welcome message, Task description...

ZhenjaHorbach commented 1 year ago

Can't we start using a new color only for left line that will be slightly different from the current one? Which will have little visual impact But we will be absolutely sure that such a case will not happen again.

hoangzinh commented 1 year ago

@ZhenjaHorbach I think no ^, we should use our existing theme colors

shawnborton commented 1 year ago

I have the same thought. But I realize we're supporting markdown on various pages I.e: Welcome message, Task description...

Can you show me examples? I just don't think we should support it in the push-to-page row, especially not the block quote style.

trjExpensify commented 1 year ago

Why aren't we just rendering this as plain text? That seems odd to me that we'd show that kind of styling in a push-to-page input. cc https://github.com/orgs/Expensify/teams/design for thoughts, as well as @techievivek @JmillsExpensify @trjExpensify

Yeah, I'm still pretty confused with how the overall implementation of this feature ended up. I mentioned it in thread, but I think we should clean it up. I agree that markdown in push rows seems a little inconsistent. I think people should ultimately be allowed to use markdown in their notes to format them, but given the design of it, we'll likely need the "live markdown" project to show it in place when you focus the note field to re-edit/add to it.

techievivek commented 1 year ago

Yeah, the issue is that we allow the users to write private notes in markdown format, so when we render it in the push-to-page pattern, it looks inconsistent as we only render plain texts there. We can go with rendering them as plain texts but then we will need another view to show the HTML-rich content.

hoangzinh commented 1 year ago

Can you show me examples? I just don't think we should support it in the push-to-page row, especially not the block quote style.

@shawnborton Sure, this is the recording

https://github.com/Expensify/App/assets/9639873/e8849533-9254-4494-838c-87616b7a0a75

shawnborton commented 1 year ago

Hmm got it. Okay, maybe it's fine that we keep the markdown in the push-to-page preview but I still think we should only change the border to use the lighter color on row hover.

peterdbarkerUK commented 1 year ago

@ZhenjaHorbach would you like to update your proposal to reflect the above?

ZhenjaHorbach commented 1 year ago

I add an alternative solution if we want to change the border color during the focus )

https://github.com/Expensify/App/issues/28858#issuecomment-1747586975

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

hoangzinh commented 1 year ago

Thanks for updating @ZhenjaHorbach. I will review it today.

hoangzinh commented 1 year ago

Thanks @shawnborton @trjExpensify @techievivek for joining the discussion on this issue. It makes our expected behavior clearer for this issue.

hoangzinh commented 1 year ago

And use RenderHTML

@ZhenjaHorbach Just wanna confirm, what is RenderHTML in your proposal? Is it this existing RenderHTML?

hoangzinh commented 1 year ago

hey @shawnborton I just checked logs and found this issue https://github.com/Expensify/App/issues/13728. I would love to hear your thoughts again on it. At the moment, the current LHN row :hover color is darker than our menu items like FAB or Setting menu items.

https://github.com/Expensify/App/assets/9639873/39f3e2d5-05b4-4bc1-8e2a-cf39bb572ee0

The LHN :hover color has been changed since this commit https://github.com/Expensify/App/commit/0f29a3992917d91193d4e64ab6fa2587038f783f (since last week). Do you still think we should update :hover color of menu items match/same with LHN row? Thanks.

ZhenjaHorbach commented 1 year ago

And use RenderHTML

@ZhenjaHorbach Just wanna confirm, what is RenderHTML in your proposal? Is it this existing RenderHTML?

Hello )

RenderHTML from library react-native-render-html) https://meliorence.github.io/react-native-render-html/api/renderhtml

This is practically the same as the RenderHTMLSource which we use https://meliorence.github.io/react-native-render-html/api/renderhtmlsource

The only difference we can pass custom styles in RenderHTML In case of RenderHTMLSource we use global styles )

Therefore, if we need custom styles We can use RenderHTML In other cases we will use RenderHTMLSource as usual

https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/components/RenderHTML.js

shawnborton commented 1 year ago

Curious for @Expensify/design 's thoughts on this question here.

I think you make a good point in that we should align on the default hover BG we use vs the BG we use when something is selected. So that being said, I do think it makes sense to use the same hover color we use in the updated LHN for all of our hoverable rows when we can.

cc @grgia in case this might be a lil PR you want to take on as I know you've expressed interest in harmonizing this kind of design stuff.

peterdbarkerUK commented 1 year ago

not overdue

dubielzyk-expensify commented 1 year ago

Curious for https://github.com/orgs/Expensify/teams/design 's thoughts on https://github.com/Expensify/App/issues/28858#issuecomment-1762747198.

I agree that I think we should update the fab menu to use the same hover color. The more subtle one is nicer imo

shawnborton commented 1 year ago

Okay cool, let's plan for that.

dannymcclain commented 1 year ago

That sounds good to me (even though I honestly do prefer the more in-your-face hover). Mega down for alignment though!

grgia commented 1 year ago

So that being said, I do think it makes sense to use the same hover color we use in the updated LHN for all of our hoverable rows when we can. @shawnborton do you mean update all rows to use this? Could you give me an example of what you mean by this. I could definitely help with this after the 22nd if not a priority this week

shawnborton commented 1 year ago

Probably not a priority for this week.

hoangzinh commented 1 year ago

If @grgia is busy, we can assign this issue to @ZhenjaHorbach, because he also mentioned a same idea to fix this issue in his proposal https://github.com/Expensify/App/issues/28858#issuecomment-1747586975

If we want to change the background color private note while hovering We can update styles here

shawnborton commented 1 year ago

That also would work for me. This would be a bigger global change applied everywhere, though it shouldn't be too hard to implement.

melvin-bot[bot] commented 1 year ago

@hoangzinh @peterdbarkerUK this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

hoangzinh commented 1 year ago

@ZhenjaHorbach I like your idea here https://github.com/Expensify/App/issues/28858#issuecomment-1747586975.

If we want to change the background color private note while hovering We can update styles here

It's also aligned with what we're going to widely update for :hover background of LHN and option menu items. Could you update your main proposal again with what @shawnborton suggested here https://github.com/Expensify/App/issues/28858#issuecomment-1766410442? Thanks

Please also refer to this issue while testing, especially this comment https://github.com/Expensify/App/issues/13728#issuecomment-1374194854.

ZhenjaHorbach commented 1 year ago

@hoangzinh

I updated the proposition ) https://github.com/Expensify/App/issues/28858#issuecomment-1747586975

And wallet works fine )

https://github.com/Expensify/App/assets/68128028/3c58dc0a-e6be-49fd-80f7-517069ebeb78

hoangzinh commented 1 year ago

@shawnborton could you elaborate on what "selected or active" means in your comment https://github.com/Expensify/App/issues/28858#issuecomment-1766410442? Does it same meaning as "selected/pressed"? In our codebase, we also have a button state called CONST.BUTTON_STATES.ACTIVE and most cases it refers to the hover state. I'm worried it will make @ZhenjaHorbach and me misunderstand your comment above.