-
Notifications
You must be signed in to change notification settings - Fork 298
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
refactor: evgeniy/92724/language settings improvements #8176
refactor: evgeniy/92724/language settings improvements #8176
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🚨 Lighthouse report for the changes in this PR:
Lighthouse ran with https://deriv-app-git-fork-yauheni-deriv-evgeniy-92724languagese-2ebeca.binary.sx/ |
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 👍
packages/account/src/Sections/Profile/LanguageSettings/language-settings.tsx
Show resolved
Hide resolved
A production App ID was automatically generated for this PR. (log)
Click here to copy & paste above information.
|
Codecov Report
@@ Coverage Diff @@
## master #8176 +/- ##
======================================
Coverage 0.05% 0.05%
======================================
Files 117 117
Lines 3425 3425
Branches 893 893
======================================
Hits 2 2
Misses 3423 3423 |
…/92724/language_settings_improvements
4ae4d10
b19746b
to
4ae4d10
Compare
…/92724/language_settings_improvements
packages/core/src/App/Components/Layout/Header/toggle-menu-drawer.jsx
Outdated
Show resolved
Hide resolved
packages/account/src/Sections/Profile/LanguageSettings/language-settings.tsx
Show resolved
Hide resolved
228311e
packages/utils/src/parse-url.ts
Outdated
/** | ||
* Checks is the url passed as prop routes to external URL resource by checking if it starts with http, https or mailto | ||
* @param link string | ||
* @returns boolean | ||
*/ | ||
export const isExternalLink = (link: string) => /^(http|https|mailto):/i.test(link); |
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.
I don't understand the use of isExternalLink
, or the variable name needs to be improved.
can we just use isNavigationFromExternalPlatform
instead.
seems like there is a typo in the comment section as well.
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.
the goal and functionality of these functions are different.
with isExternalLink we are checking additionally if the route is to external link and not to the some of the app's page. Basically it i used to resolve our current production issue, when you try to navigate through account settings for responsive version, you see reloading the app. It is due to wrong usage of and .
so this issue has been fixed in this PR.
will fix the typo 👍
9a8d7e3
to
32188d5
Compare
…/92724/language_settings_improvements
…/92724/language_settings_improvements
…/92724/language_settings_improvements
…/92724/language_settings_improvements
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
* refactor: evgeniy/92724/language settings improvements * fix: cursor button fix * feat: same language menu usage for responsive * fix: hide language icon while opening language menu * refactor: review change, adding types and dependencies * fix: changes from master missed * fix: reading undefined error * fix: duplicated properties * fix: types duplicate * chore: language-settings test * refactor: menu-link store separately * chore: menu-link test coverage * refactor: test code structure * refactor: link code * refactor: remove unused code * refactor: code readability * refactor: variable usage * refactor: variable usage * refactor: review comments incorporating * fix: test refactor with mockstore * fix: missing mock * fix: 🎨 incorporated deriv/util package * fix: ⚡ refactored code * feat: resolved code smells * feat: resolved review comments * feat: resolved review comments * feat: resolved review comments * fix: failing testcase * fix: 🐛 missing import * Trigger build * chore: trigger build * Merge branch 'master' into evgeniy/92724/language_settings_improvements * chore: review comments incorporate * refactor: types store alphabetically order * refactor: types store alphabetically order * refactor: menulink export fix * chore: remove unused variable * refactor: remove unused variable * chore: trigger build * fix: resolve conflicts * fix: repeated fields * chore: isExternalLink description * chore: bootstrap --------- Co-authored-by: “yauheni-kryzhyk-deriv” <“yauheni@deriv.me”> Co-authored-by: Likhith Kolayari <98398322+likhith-deriv@users.noreply.github.com> Co-authored-by: Matin shafiei <matin@deriv.com> Co-authored-by: Likhith Kolayari <likhith@regentmarkets.com>
* refactor: evgeniy/92724/language settings improvements * fix: cursor button fix * feat: same language menu usage for responsive * fix: hide language icon while opening language menu * refactor: review change, adding types and dependencies * fix: changes from master missed * fix: reading undefined error * fix: duplicated properties * fix: types duplicate * chore: language-settings test * refactor: menu-link store separately * chore: menu-link test coverage * refactor: test code structure * refactor: link code * refactor: remove unused code * refactor: code readability * refactor: variable usage * refactor: variable usage * refactor: review comments incorporating * fix: test refactor with mockstore * fix: missing mock * fix: 🎨 incorporated deriv/util package * fix: ⚡ refactored code * feat: resolved code smells * feat: resolved review comments * feat: resolved review comments * feat: resolved review comments * fix: failing testcase * fix: 🐛 missing import * Trigger build * chore: trigger build * Merge branch 'master' into evgeniy/92724/language_settings_improvements * chore: review comments incorporate * refactor: types store alphabetically order * refactor: types store alphabetically order * refactor: menulink export fix * chore: remove unused variable * refactor: remove unused variable * chore: trigger build * fix: resolve conflicts * fix: repeated fields * chore: isExternalLink description * chore: bootstrap --------- Co-authored-by: “yauheni-kryzhyk-deriv” <“yauheni@deriv.me”> Co-authored-by: Likhith Kolayari <98398322+likhith-deriv@users.noreply.github.com> Co-authored-by: Matin shafiei <matin@deriv.com> Co-authored-by: Likhith Kolayari <likhith@regentmarkets.com>
Changes:
Please include a summary of the change and which issue is fixed below:
When you need to add unit test
When you need to add integration test
Test coverage checklist (for reviewer)
Type of change