melvin-bot[bot]
commented
10 months ago Job added to Upwork: https://www.upwork.com/jobs/~01817cdb0a4cd143cf
Closed izarutskaya closed 9 months ago
melvin-bot[bot]
commented
10 months ago Job added to Upwork: https://www.upwork.com/jobs/~01817cdb0a4cd143cf
melvin-bot[bot]
commented
10 months ago Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
melvin-bot[bot]
commented
10 months ago Platforms in OP are ✅)melvin-bot[bot]
commented
10 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)
aswin-s
commented
10 months ago New workspace icon on FAB menu is not changing its color on hover.
It seems like the intension is to keep the icon as such as displayInDefaultIconColor: true is set for this icon. Also the SVG used for this icon has fill color defined in it which prevents the fill from being modified programatically.
If we infact want to change the icon color on hover displayInDefaultIconColor: true should be removed from the icon for newWorkspace. Also to fix the workspace icon for both light and dark themes below changes should be made.
Modify MenuItem.tsx like below
<View style={[styles.popoverMenuIcon, iconStyles, StyleUtils.getAvatarWidthStyle(avatarSize), { color: theme.iconReversed }]}>Also the svg should be modified to remove the fill Color and use currentColor for building icon.
<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
https://github.com/Expensify/App/assets/6880914/b63ae4cc-97b7-45f8-b65f-a983cfffb06b
None (edited)
sofi-a
commented
10 months ago The "New workspace" menu icon under the FAB menu doesn't change colors when hovered on.
It looks like the icon was intentionally made to not change colors as the displayInDefaultIconColor property is set to true here.
Removing the displayInDefaultIconColor property is not enough in this case because the new-workspace icon is more complex than all the other menu icons under the FAB menu. My solution is to create a react component version of the icon to maximize the ease of customization.
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 displayInDefaultIconColor property should either be removed or set to false.
The new menu item object should look something like this.
{
contentFit: 'contain',
icon: NewWorkspaceIcon,
iconWidth: 46,
...React 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 src.
https://github.com/Expensify/App/assets/31987492/0dd62fd4-552b-45b3-a138-2e7fc6a04eb1
https://github.com/Expensify/App/assets/31987492/ec92cdfa-da58-49fe-a66e-434bd9b2c34b
N/A
melvin-bot[bot]
commented
10 months ago @bfitzexpensify, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
getusha
commented
10 months ago the proposal from @aswin-s looks good to me, seems like an issue we experienced before on other icons. 🎀 👀 🎀 C+ Reviewed!
melvin-bot[bot]
commented
10 months ago Triggered auto assignment to @MariaHCD, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
sofi-a
commented
10 months ago @getusha The color of the building in the icon in dark mode is currently white. @aswin-s's proposal would make it black even when it's not hovered on. Is that desired?
Screenshot from staging
aswin-s
commented
10 months ago @sofi-a @getusha I simply used the fill style for other icons like request money. If we want to retain building color as white for both dark and light mode, currentColor in SVG could be set accordingly. It's just an implementation detail.
melvin-bot[bot]
commented
10 months ago 📣 @getusha 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
melvin-bot[bot]
commented
10 months ago 📣 @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 Upwork job Please accept the offer 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 📖
Pujan92
commented
10 months ago 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
aswin-s
commented
10 months ago @Pujan92 Expensicons.LoungeAccess icon is throwing error on UserProfile Page. I guess it is not used at the moment.
Try removing props.user.hasLoungeAccess conditional and load the page on a native device.
The MenuItem component passes the icon prop down to ImageSVG component, which uses expo-image library to display the SVG file. It cannot accept a react component as mentioned in @sofi-a 's proposal. That's also the reason why UserProfile page crash if you try to enable the Lounge Access menu item.
Pujan92
commented
10 months ago 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. Maybe the design team can help here about omitting the stroke around the star is fine or not @shawnborton
| Current | With selected proposal |
|---|---|
aswin-s
commented
10 months ago @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.
| Normal State | Hovered State |
|---|---|
shawnborton
commented
10 months ago 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 ?
shawnborton
commented
10 months ago 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?
aswin-s
commented
10 months ago 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.
shawnborton
commented
10 months ago Ah okay, so what exactly is the bug then or what do you need the design team's input on?
aswin-s
commented
10 months ago 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.
shawnborton
commented
10 months ago Got it, sounds good! Let's not remove the stroke.
dannymcclain
commented
10 months ago 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.
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!
shawnborton
commented
10 months ago So this one isn't actually a bug - can we just close the issue out? cc @MariaHCD @bfitzexpensify
MariaHCD
commented
10 months ago Thanks for the input on the design here! I agree that this is not a bug.
aswin-s
commented
10 months ago @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?
getusha
commented
9 months ago @MariaHCD I think partial compensation makes sense, since there was time spent on the PR before deciding to close this out.
cc @bfitzexpensify
MariaHCD
commented
9 months ago 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?
aswin-s
commented
9 months ago @MariaHCD I leave the decision to you. That's why I was checking if there is a process already in place for such scenarios.
MariaHCD
commented
9 months ago getusha
commented
9 months ago bump @bfitzexpensify
bfitzexpensify
commented
9 months 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!
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
https://github.com/Expensify/App/assets/115492554/d2df22a0-a94b-4509-8cec-2ac6265e3d98
View all open jobs on GitHub
Upwork Automation - Do Not Edit