-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Temporarily returns "https://" as the default website #47314
Temporarily returns "https://" as the default website #47314
Conversation
@paultsimura Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@paultsimura @suneox will review this. |
Looks like Github had some problems this morn, but it should be resolved now - @suneox are you able to test this soon? Since this is fixing a deploy blocker it would be great to prioritize this 🙏 |
Sorry for the delay. I’ll be home in about 2 hours and can start the review. |
might need to pull |
Merged with main |
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.
@bernhardoj NAB since this is temporary but i think you can just flat out remove the commented out sections (aside from the explanation comment) since we'll probably just straight revert this once the other issue is resolved.
I'm comeback and start review now I will complete test & the checklist about 20 minutes |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2024-08-13.at.23.57.16.movScreen.Recording.2024-08-14.at.00.00.54.movMacOS: DesktopScreen.Recording.2024-08-13.at.23.55.19.movAndroid: NativeScreen.Recording.2024-08-14.at.00.11.29.movScreen.Recording.2024-08-14.at.00.09.43.movAndroid: mWeb ChromeScreen.Recording.2024-08-14.at.00.06.38.moviOS: NativeScreen.Recording.2024-08-14.at.00.05.47.moviOS: mWeb SafariScreen.Recording.2024-08-14.at.00.01.32.mov |
function getDefaultCompanyWebsite(session: OnyxEntry<OnyxTypes.Session>, user: OnyxEntry<OnyxTypes.User>): string { | ||
return user?.isFromPublicDomain ? 'https://' : `https://www.${Str.extractEmailDomain(session?.email ?? '')}`; | ||
// return user?.isFromPublicDomain ? 'https://' : `https://www.${Str.extractEmailDomain(session?.email ?? '')}`; |
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.
NAB: Instead of make a block code comment, we can create a constant with description
// return user?.isFromPublicDomain ? 'https://' : `https://www.${Str.extractEmailDomain(session?.email ?? '')}`; | |
// temporarily always returns https:// to fix https://github.com/Expensify/App/issues/47227 until https://github.com/Expensify/App/issues/45278 is resolved. | |
const isPending = true; | |
if (isPending) { | |
return 'https://'; | |
} |
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.
Tested successfully with a private email by domain and gmail account
closing this one in favour of #47345 |
Details
We want to temporarily return "https://" as the default website to unblock #47227 until #45278 is resolved.
Fixed Issues
$ #47227
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
web.mp4