Skip to content

Allow editing of Workspace names#4298

Merged
kevinksullivan merged 19 commits intomainfrom
horus-edit-workspace-name
Aug 2, 2021
Merged

Allow editing of Workspace names#4298
kevinksullivan merged 19 commits intomainfrom
horus-edit-workspace-name

Conversation

@HorusGoul
Copy link
Contributor

@HorusGoul HorusGoul commented Jul 29, 2021

Details

Adds an edit button at the right side of the workspace name that opens a modal where you can change the workspace name.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/171962

Tests/QA Steps

  1. Create or open a workspace
  2. Click the shown in along with the avatar
  3. A modal with an avatar editor and a name input should open.
  4. In that modal, change everything and press "Save"
  5. The modal should close, and everything you changed in the modal should get updated everywhere.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2021-08-02.at.09.50.02.mov

Mobile Web

Screen.Recording.2021-08-02.at.09.54.38.mov

Desktop

Screen.Recording.2021-08-02.at.09.50.02.mov

iOS

Screen.Recording.2021-08-02.at.09.43.59.mov

Android

Screen.Recording.2021-08-02.at.09.48.46.mov

@HorusGoul HorusGoul self-assigned this Jul 29, 2021
@HorusGoul HorusGoul marked this pull request as ready for review July 29, 2021 12:48
@HorusGoul HorusGoul requested a review from a team as a code owner July 29, 2021 12:48
@MelvinBot MelvinBot requested review from mountiny and removed request for a team July 29, 2021 12:48
@kevinksullivan
Copy link
Contributor

@HorusGoul @MitchExpensify I don't think we want to add another pencil icon. Let's keep the one pencil icon, and then when a user selects that we open this right pane, which allows a user to edit their workspace avatar OR their workspace name.

image

@kevinksullivan kevinksullivan self-requested a review July 29, 2021 14:26
@mountiny
Copy link
Contributor

Was just testing it and I agree with @kevinksullivan, that will be cleared, two same icons right next to each other seem weird.

@kevinksullivan
Copy link
Contributor

@shawnborton I am also curious for your thoughts on this one. I think we've discussed the idea of a business overview page for the purpose of editing name, avatar, business address, etc... But in the meantime, I think it is ok to stick with a single pencil icon that opens the right pane for editing avatar and/or name.

@shawnborton
Copy link
Contributor

I agree with that sentiment Kevin.

Separately, I wonder if you wouldn't mind making some quick tweaks to the pencil icon/circle that's over the workspace icon? Ideally it would look more like this:
image

  • The gray circle is 28x28
  • The pencil icon is 20x20
  • The circle is -4 from the bottom edge and -4 from the right edge

@HorusGoul
Copy link
Contributor Author

Awesome, I'll move the avatar editor inside the modal and leave only 1 pencil icon.

Separately, I wonder if you wouldn't mind making some quick tweaks to the pencil icon/circle that's over the workspace icon?

Sure!

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Jul 29, 2021

Completely agree after seeing the mock! So much better

mountiny
mountiny previously approved these changes Jul 29, 2021
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't spotted any blockers in the code! Tested and works well 🎉 Great job!

@HorusGoul
Copy link
Contributor Author

Haven't spotted any blockers in the code! Tested and works well 🎉 Great job!

I just found this little issue regarding the pencil button on iOS. I think it's related to the border being drawn inside the circle instead of outside:

image

Maybe that's why we had the other width/height values before. I'm gonna send another commit with a fix.

@mountiny
Copy link
Contributor

Great catch! I am just wondering whether the Avatar change flow is not getting too hidden with this change. But that is not an issue now I guess, we will find out based on user feedback if they struggle to change the avatar.

@mountiny mountiny self-requested a review July 29, 2021 16:43
@HorusGoul
Copy link
Contributor Author

It should be ready for review again, and all videos have been updated in case you want to take a look!

mountiny
mountiny previously approved these changes Jul 29, 2021
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that small problem. LGTM

@shawnborton
Copy link
Contributor

Looks good, thanks!

onChangeText={name => this.setState({name})}
onSubmitEditting={this.submit}
/>
<Text style={[styles.mt2]}>{this.props.translate('workspace.editor.nameInputHelpText')}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also use fontSizeLabel and colorMuted here? This way it appears like hint text under the input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there's a formHint style that has those! I just sent the commit with the changes 😄

@shawnborton
Copy link
Contributor

Actually after looking at this a second time, I wonder if it feels weird that you see the avatar image with a pencil icon twice. Maybe for consistency's sake, we should remove the pencil icon from the first screen and just make it so that tapping the avatar or the workspace name will launch the rightDocked modal. From here, I would expect the avatar to have the pencil icon on it. This way we stay more consistent with say editing your profile, where you have to first click on your avatar image (with no pencil icon).

@kevinksullivan
Copy link
Contributor

Yeah I agree with Shawn. Showing the pencil icon twice feels like overkill here, and I like keeping user avatar and workspace avatar designs consistent.

@HorusGoul
Copy link
Contributor Author

HorusGoul commented Jul 30, 2021

we should remove the pencil icon from the first screen and just make it so that tapping the avatar or the workspace name will launch the rightDocked modal.

Tapping the avatar or the workspace name to access the editor could look weird, as there would be no visual hints for the user. If we want to make this consistent with the user profile editor, we need to add a new menu option like this one:

What do you think?

@mountiny
Copy link
Contributor

That is a good point, at the same time, I would say it is not very uncommon from other applications and UIs that you click on the avatar/name to change it (Facebook for example).

What I would suggest is instead of adding another Profile or its equivalent for Workspace, make the User Avatar and name Pressable too in the Settings panel and it will lead to the Profile page.

This will ensure a consistency between these two without over-engineering the Workspace page. What do you think? 👀

@HorusGoul
Copy link
Contributor Author

I just submitted the changes to make it that way for both the User profile and the Workspace profile. I've also updated the videos 😄

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, tested! Love this flow.

@kevinksullivan kevinksullivan merged commit 588700e into main Aug 2, 2021
@kevinksullivan kevinksullivan deleted the horus-edit-workspace-name branch August 2, 2021 13:27
@OSBotify
Copy link
Contributor

OSBotify commented Aug 2, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants