-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Concierge icon #12425
Update Concierge icon #12425
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
As fas as merging this one goes, I wonder if we want to hold and try to merge some of the color/brand changes all in the same day so we try to group them in the same deploy? Thoughts? |
Got assigned to this one. Straight forward PR. 🎀 👀 🎀
ScreenshotsWebMobile Web - ChromeMobile Web - SafariDesktopiOSAndroid |
Fine with holding this if that's what you'd prefer @shawnborton! |
Let's hold off for a day or so, I'll start up some discussion on what the best way is to handle these changes. I kind of feel like it would be nice to group together the following changes:
|
Do we plan to merge this ? Do we have any linked PRs which we should track together? |
Yup, let's still hold on this. I think we want to group this with these at least: |
@shawnborton looks like we've merged some of the other updates you listed above. Should we take this off hold now? Is there a particular change we're waiting on? |
Great reminder - I agree, let's get this merged now! |
🚀 Deployed to staging by @NikkiWines in version: 1.2.33-0 🚀
|
1 similar comment
🚀 Deployed to staging by @NikkiWines in version: 1.2.33-0 🚀
|
🚀 Deployed to staging by @NikkiWines in version: 1.2.33-0 🚀
|
🚀 Deployed to production by @luacmartins in version: 1.2.33-7 🚀
|
🚀 Deployed to production by @luacmartins in version: 1.2.33-7 🚀
|
cc: @shawnborton
Details
Updates the current concierge icon to use our new icon in NewDot only.
Removes an old concierge image that's unused.
Fixed Issues
$ #12264
Tests
QA Steps
PR Author Checklist
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots
OldDot screenshot showing the icon is the same:
![oldDot](https://user-images.githubusercontent.com/16747822/199721166-f9f42e02-63a7-44b7-9b63-86189cce1a33.png)
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android