[$500] FAB - Menu icon doesn't change color when hovered over "New workspace" in FAB menu #33747
Comments
izarutskaya
added
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~01817cdb0a4cd143cf |
|
Triggered auto assignment to @bfitzexpensify ( |
melvin-bot
bot
added
the
Help Wanted
Bug0 Triage Checklist (Main S/O)
|
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.New workspace icon on FAB menu is not changing its color on hover. What is the root cause of that problem?It seems like the intension is to keep the icon as such as What changes do you think we should make in order to solve the problem?If we infact want to change the icon color on hover Modify MenuItem.tsx like below App/src/components/MenuItem.tsx Line 433 in 5c5c862 <View style={[styles.popoverMenuIcon, iconStyles, StyleUtils.getAvatarWidthStyle(avatarSize), { color: theme.iconReversed }]}>Also the svg should be modified to remove the fill Color and use <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" id="Layer_1" x="0" y="0" version="1.1" viewBox="0 0 40 40" style="enable-background:new 0 0 40 40" xml:space="preserve"><style type="text/css">.st0{}.st1{fill-rule:evenodd;clip-rule:evenodd;fill:currentColor}.st2{fill:#fed607;stroke-width:2;stroke-linecap:round;stroke-linejoin:round}</style><path d="M20,0L20,0c11,0,20,9,20,20l0,0c0,11-9,20-20,20l0,0C9,40,0,31,0,20l0,0C0,9,9,0,20,0z" class="st0"/><path d="M13,12.8c0-1,0.8-1.8,1.8-1.8h10.5c1,0,1.8,0.8,1.8,1.8v14.5c0,1-0.8,1.8-1.8,1.8H22v-3.2 c0-0.4-0.4-0.8-0.8-0.8h-2.4c-0.4,0-0.8,0.4-0.8,0.8V29h-3.2c-1,0-1.8-0.8-1.8-1.8V12.8z M16.9,13c-0.6,0-1,0.4-1,1s0.4,1,1,1h6.2 c0.6,0,1-0.4,1-1s-0.4-1-1-1H16.9z M15.9,18c0-0.6,0.4-1,1-1h6.2c0.6,0,1,0.4,1,1s-0.4,1-1,1h-6.2C16.3,19,15.9,18.6,15.9,18z M16.9,21c-0.6,0-1,0.4-1,1s0.4,1,1,1h6.2c0.6,0,1-0.4,1-1s-0.4-1-1-1H16.9z" class="st1"/><path d="M28.8,5.8C28.8,5.7,28.7,5.7,28.8,5.8c-0.6-1-2-1-2.6,0l0,0l0,0l-0.9,1.7l-2.1,0.3l0,0l0,0 c-1.1,0.1-1.8,1.5-0.8,2.4l0,0l1.5,1.3l-0.3,1.8v0c-0.1,0.6,0.2,1.1,0.6,1.4c0.4,0.3,1,0.3,1.5,0.1c0,0,0,0,0,0l1.9-0.9l1.9,0.9 c0,0,0,0,0,0c0,0,0,0,0,0c0.5,0.2,1.1,0.2,1.5-0.1c0.4-0.3,0.7-0.8,0.6-1.4v0l-0.3-1.8l1.5-1.3l0,0c1-0.9,0.3-2.3-0.8-2.4l0,0 l-2.1-0.3L28.8,5.8z" class="st2"/></svg>
Result theme.mp4What alternative solutions did you explore? (Optional)None (edited) |
ProposalPlease re-state the problem that we are trying to solve in this issue.The "New workspace" menu icon under the FAB menu doesn't change colors when hovered on. What is the root cause of that problem?It looks like the icon was intentionally made to not change colors as the What changes do you think we should make to solve the problem?Removing the import React from 'react';
import Svg, {Path} from 'react-native-svg';
import useThemePreference from '@hooks/useThemePreference';
type NewWorkspaceIconProps = {
/** The fill color for the icon. Can be hex, rgb, rgba, or valid react-native named color such as 'red' or 'blue'. */
fill?: string;
/** Is icon hovered */
hovered?: string;
/** Is icon pressed */
pressed?: string;
/** Icon's width */
width?: number;
/** Icon's height */
height?: number;
};
function NewWorkspaceIcon(props: NewWorkspaceIconProps) {
const themePreference = useThemePreference();
const isInteracting = props.hovered === "true" || props.pressed === "true";
const backgroundColor = (isInteracting && props.fill) || "#03d47c";
const buildingColor =
isInteracting && themePreference === "dark" ? "#000" : "#fff";
return (
<Svg
x="0"
y="0"
viewBox="0 0 40 40"
style={{enableBackground: 'new 0 0 40 40'}}
xmlSpace="preserve"
{...props}
>
{/* Background */}
<Path
d="M20,0L20,0c11,0,20,9,20,20l0,0c0,11-9,20-20,20l0,0C9,40,0,31,0,20l0,0C0,9,9,0,20,0z"
fill={backgroundColor}
/>
{/* Building */}
<Path
d="M13,12.8c0-1,0.8-1.8,1.8-1.8h10.5c1,0,1.8,0.8,1.8,1.8v14.5c0,1-0.8,1.8-1.8,1.8H22v-3.2 c0-0.4-0.4-0.8-0.8-0.8h-2.4c-0.4,0-0.8,0.4-0.8,0.8V29h-3.2c-1,0-1.8-0.8-1.8-1.8V12.8z M16.9,13c-0.6,0-1,0.4-1,1s0.4,1,1,1h6.2 c0.6,0,1-0.4,1-1s-0.4-1-1-1H16.9z M15.9,18c0-0.6,0.4-1,1-1h6.2c0.6,0,1,0.4,1,1s-0.4,1-1,1h-6.2C16.3,19,15.9,18.6,15.9,18z M16.9,21c-0.6,0-1,0.4-1,1s0.4,1,1,1h6.2c0.6,0,1-0.4,1-1s-0.4-1-1-1H16.9z"
fillRule="evenodd"
clipRule="evenodd"
fill={buildingColor}
/>
{/* Star */}
<Path
d="M28.8,5.8C28.8,5.7,28.7,5.7,28.8,5.8c-0.6-1-2-1-2.6,0l0,0l0,0l-0.9,1.7l-2.1,0.3l0,0l0,0 c-1.1,0.1-1.8,1.5-0.8,2.4l0,0l1.5,1.3l-0.3,1.8v0c-0.1,0.6,0.2,1.1,0.6,1.4c0.4,0.3,1,0.3,1.5,0.1c0,0,0,0,0,0l1.9-0.9l1.9,0.9 c0,0,0,0,0,0c0,0,0,0,0,0c0.5,0.2,1.1,0.2,1.5-0.1c0.4-0.3,0.7-0.8,0.6-1.4v0l-0.3-1.8l1.5-1.3l0,0c1-0.9,0.3-2.3-0.8-2.4l0,0 l-2.1-0.3L28.8,5.8z"
fill="#fed607"
stroke={backgroundColor}
strokeWidth={2}
strokeLinecap="round"
strokeLinejoin="round"
/>
</Svg>
);
}
export default NewWorkspaceIcon;All color customization logic is encapsulated in this component ensuring that we don't have to change how the MenuItem component works or alter the svg. This is important to avoid affecting other parts of the platform. Import the new icon in FloatingActionButtonAndPopover. import NewWorkspaceIcon from '@components/Icon/svgs/NewWorkspaceIcon';Then replace the reference to Expensicons.NewWorkspace with the new icon. The The new menu item object should look something like this. {
contentFit: 'contain',
icon: NewWorkspaceIcon,
iconWidth: 46,
...CompatibilityReact components aren't supported by expo-image and hence my proposed changes won't work on native platforms. To get around this issue, the native ImageSVG component should be updated. import {Image} from 'expo-image';
import React from 'react';
import type {ImageSourcePropType} from 'react-native';
import type {SvgProps} from 'react-native-svg';
import type ImageSVGProps from './types';
function ImageSVG({
src,
width = '100%',
height = '100%',
fill,
contentFit = 'cover',
hovered = false,
pressed = false,
style,
pointerEvents,
preserveAspectRatio
}: ImageSVGProps) {
const tintColorProp = fill ? {tintColor: fill} : {};
if (typeof src === 'function') {
const ImageSvgComponent = src as React.FC<SvgProps>;
const additionalProps: Pick<ImageSVGProps, 'fill' | 'pointerEvents' | 'preserveAspectRatio'> = {};
if (fill) {
additionalProps.fill = fill;
}
if (pointerEvents) {
additionalProps.pointerEvents = pointerEvents;
}
if (preserveAspectRatio) {
additionalProps.preserveAspectRatio = preserveAspectRatio;
}
return (
<ImageSvgComponent
width={width}
height={height}
fill={fill}
hovered={`${hovered}`}
pressed={`${pressed}`}
style={style}
{...additionalProps}
/>
)
}
return (
<Image
contentFit={contentFit}
source={src as ImageSourcePropType}
style={[{width, height}, style]}
// eslint-disable-next-line react/jsx-props-no-spreading
{...tintColorProp}
/>
);
}
ImageSVG.displayName = 'ImageSVG';
export default ImageSVG;It's essentially a duplication of the default ImageSVG component when a function is provided as the ResultScreen.Recording.2023-12-31.at.10.11.35.movScreen.Recording.2024-01-08.at.22.55.54.movWhat alternative solutions did you explore? (Optional)N/A |
|
@bfitzexpensify, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
|
Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @getusha 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
|
📣 @aswin-s 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
melvin-bot
bot
added
Reviewing
|
Just sharing my opinion that we also have the stroke width around the star(which I believe is given to prevent overlap between center and star icon to look good) and that color matches with the background color, isn't @sofi-a's proposal more accurate as we are also doing the same for Lounge Icon here |
|
@Pujan92
Try removing The MenuItem component passes the icon prop down to |
|
I got your point that it will crash at the moment after expo-image changes, to solve we can render conditionally considering whether src is a function or not which won't render in expo image if it is a function.
|
|
@Pujan92 I never had the intension to remove the stroke around the star and I didn't mention anything like that in the proposal. Just that when I made the fix I accidentally dropped the stroke. I've fixed that now in the PR. Thanks for noticing. This is how it looks now.
|
|
Part of me thinks we should keep the green BG on this particular item on hover, it feels a bit more special than the other items and almost like we want to specifically promote the green BG here as a new item to discover. What do you think @dannymcclain @dubielzyk-expensify ? |
|
As far as the stroke goes, we want to keep it there, but we can recreate the svg to not actually have a stroke line and just subtract that outline from the building shape. Is that what you need? |
@shawnborton That's how it is achieved right now. So we wouldn't be needing another SVG file unless you feel something is missing with the current view. And there is no plan to remove the stroke.
|
|
Ah okay, so what exactly is the bug then or what do you need the design team's input on? |
In the first implementation I accidentally missed the stroke. So @Pujan92 thought it was on purpose and pinged the design team to check whether it is okay to remove the stroke. I've clarified above that it was never the intention and just a mistake. Hope that clears all the confusion. |
|
Got it, sounds good! Let's not remove the stroke. |
I tend to agree. I don't really see this is as a bug—more of a choice. The "new workspace" icon feels intentionally different from the rest (and it's also the only item with a description, which further makes it feel special), and the row still gets a hover state, so I don't really see the need to change the icon for this one. BUT if y'all feel really strongly that we do need to change it, I won't stand in the way! |
|
So this one isn't actually a bug - can we just close the issue out? cc @MariaHCD @bfitzexpensify |
|
Thanks for the input on the design here! I agree that this is not a bug. |
|
@MariaHCD Considerable effort was put into the PR to make it work seamlessly against both web and native platforms. The expected outcome on both themes were detailed out in the proposal itself. Still it got assigned and PR was raised 2 weeks ago. Is there a policy regarding how to compensate contributors on issues which gets closed after assigning and PR was raised? |
|
@MariaHCD I think partial compensation makes sense, since there was time spent on the PR before deciding to close this out. |
|
Okay, that's fair. I'd like @bfitzexpensify's input but what partial compensation here do you think is fair in this case, @aswin-s? |
|
@MariaHCD I leave the decision to you. That's why I was checking if there is a process already in place for such scenarios. |
|
Referencing the internal SO and this doc: Considering the effort/time spent on the PR and on testing as well as the C+'s time in reviewing the proposals here and reviewing the PR, I think 25% partial payment to both is fair. Thoughts, @bfitzexpensify? |
|
bump @bfitzexpensify |
I agree, 25% seems fair. Payments complete. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: v1.4.19-0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause-Internal Team
Slack conversation: @
Action Performed:
Expected Result:
"New workspace" logo icon should change to black color in light theme & to white in dark theme when hovered over the option
Actual Result:
"New workspace" logo icon doesn't change color when hovered over the menu
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Bug6328100_1703819592401.Recording__11.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: