Skip to content
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

Replace smart dash with double hyphen #14419

Merged
merged 5 commits into from
Jan 23, 2023
Merged

Replace smart dash with double hyphen #14419

merged 5 commits into from
Jan 23, 2023

Conversation

luacmartins
Copy link
Contributor

Details

Prevents iOS from converting double hyphens to a "smart" dash.

Fixed Issues

$ #14137

Tests

  1. Navigate to + > New room
  2. Enter a room name with two consecutive hyphens
  3. Tap outside the Room name text input
  4. Verify that you don't see an error
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

N/A

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
android.mov

@luacmartins luacmartins self-assigned this Jan 19, 2023
@luacmartins luacmartins marked this pull request as ready for review January 19, 2023 22:01
@luacmartins luacmartins requested a review from a team as a code owner January 19, 2023 22:01
@melvin-bot melvin-bot bot requested review from mollfpr and techievivek and removed request for a team January 19, 2023 22:01
@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

@mollfpr @techievivek One of you needs to 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]

@mollfpr
Copy link
Contributor

mollfpr commented Jan 20, 2023

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
14419.Web.mov
Mobile Web - Chrome
14419.mWeb-Chrome.mov
Mobile Web - Safari
14419.mWeb-Safari.mov
Desktop
14419.Desktop.mov
iOS
14419.iOS.mov
Android
14419.Android.mov

Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

LGTM and test well 👍

Copy link
Contributor

@techievivek techievivek left a comment

Choose a reason for hiding this comment

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

Looks good.

@techievivek techievivek merged commit a2dab61 into main Jan 23, 2023
@techievivek techievivek deleted the cmartins-fixRoomInput branch January 23, 2023 06:54
@OSBotify
Copy link
Contributor

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

@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 692.452 ms → 703.001 ms (+10.549 ms, +1.5%)
Open Search Page TTI 606.932 ms → 607.262 ms (+0.331 ms, ±0.0%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +7.7%)
App start nativeLaunch 20.500 ms → 19.677 ms (-0.823 ms, -4.0%)
App start runJsBundle 204.844 ms → 198.581 ms (-6.263 ms, -3.1%)
Show details
Name Duration
App start TTI Baseline
Mean: 692.452 ms
Stdev: 38.343 ms (5.5%)
Runs: 603.3139720000327 612.3992599993944 637.2739829998463 651.3543180003762 659.6461980007589 663.9311599992216 666.221549000591 666.2972119990736 667.4444980006665 669.5687269996852 683.7036269996315 684.5362250003964 687.8337489999831 687.9909109994769 688.9775900002569 691.0918829999864 693.3654100000858 694.4465800002217 695.4093169998378 697.8343860004097 698.5036019999534 705.1892290003598 705.703343000263 713.5359160006046 714.6525599993765 718.8641320001334 722.9302409999073 741.1444269996136 747.6472600009292 750.9171370007098 766.4606020003557 770.289015000686

Current
Mean: 703.001 ms
Stdev: 30.611 ms (4.4%)
Runs: 654.2403909992427 660.765365999192 666.7419520001858 668.486370999366 673.1781949996948 678.9088710006326 679.8913070000708 683.0153039991856 683.2129590008408 683.7824009992182 684.1047719996423 684.3557159993798 687.7435829993337 689.3368020001799 693.594946000725 695.1417740006 695.4625470004976 698.9282409995794 699.2749429997057 699.4244780000299 699.9945950005203 708.7850110009313 722.885816000402 727.2671789992601 728.5716680008918 731.4435920007527 740.543959999457 740.9207150004804 743.2805010005832 745.5152800008655 769.2817349992692 777.9659160003066
Open Search Page TTI Baseline
Mean: 606.932 ms
Stdev: 24.815 ms (4.1%)
Runs: 570.8819989990443 578.2485770005733 578.9293619990349 580.9259029999375 581.3488769996911 582.105835000053 583.2759200017899 583.625732999295 583.9208579994738 590.4658610001206 591.7648120000958 591.816366000101 593.8699949998409 596.7663170006126 601.2271330002695 601.9165850002319 603.8301190007478 604.9570319987833 608.215779999271 613.860555998981 615.0572920013219 616.5614829994738 617.1390380002558 619.5515139997005 620.8560790009797 624.055622998625 626.5093590002507 637.8711349982768 640.6890869997442 653.2436529994011 662.113404000178 666.2211910001934

Current
Mean: 607.262 ms
Stdev: 26.587 ms (4.4%)
Runs: 562.1555579993874 576.1888430006802 576.96529099904 580.043171999976 580.5448010005057 580.9579670000821 581.0985919982195 582.7352300006896 586.4650880005211 588.7445880006999 589.4916999991983 592.5936689991504 599.12231499888 600.5463459994644 601.3052169997245 605.5916760005057 607.3308919984847 608.0965170003474 612.6982829999179 612.9250900000334 613.6747639998794 616.5402840003371 616.7405609991401 618.9899909999222 619.76639899984 620.5760909989476 624.7460530009121 634.5649419985712 637.9174809996039 662.7864589989185 665.015625 675.4792890008539
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.7%)
Runs: 0.012085000053048134 0.012654999271035194 0.012736000120639801 0.012817999348044395 0.01285799965262413 0.01314300112426281 0.013224000111222267 0.013304999098181725 0.013345999643206596 0.013346999883651733 0.013469001278281212 0.013631001114845276 0.01371300034224987 0.013793999329209328 0.013833999633789062 0.013916000723838806 0.013916000723838806 0.013956999406218529 0.01407800056040287 0.014159999787807465 0.014159999787807465 0.014241999015212059 0.014241999015212059 0.014322999864816666 0.014364000409841537 0.014526000246405602 0.014608001336455345 0.014689000323414803 0.014770999550819397 0.015137000009417534 0.01586899906396866

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.7%)
Runs: 0.013345999643206596 0.013753000646829605 0.013753000646829605 0.013753999024629593 0.013876000419259071 0.013916000723838806 0.013996999710798264 0.014118999242782593 0.014161000028252602 0.014200998470187187 0.014281999319791794 0.01428299956023693 0.014404000714421272 0.01448499970138073 0.014607999473810196 0.014649000018835068 0.014770999550819397 0.014852000400424004 0.015014998614788055 0.015095999464392662 0.015217998996376991 0.01521800085902214 0.01533999852836132 0.01534000039100647 0.015583999454975128 0.015584001317620277 0.015829000622034073 0.01619499921798706 0.01631700061261654 0.016683001071214676 0.017089998349547386 0.017333999276161194
App start nativeLaunch Baseline
Mean: 20.500 ms
Stdev: 2.318 ms (11.3%)
Runs: 17 18 18 18 18 18 18 18 18 19 19 19 20 20 20 20 21 21 21 21 21 21 22 22 22 22 22 22 23 25 26 26

Current
Mean: 19.677 ms
Stdev: 1.329 ms (6.8%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 21 21 21 21 21 22 22 23
App start runJsBundle Baseline
Mean: 204.844 ms
Stdev: 24.945 ms (12.2%)
Runs: 163 164 175 176 178 180 184 186 187 190 192 194 195 197 197 201 203 203 209 212 214 216 217 218 221 225 229 230 234 234 261 270

Current
Mean: 198.581 ms
Stdev: 20.381 ms (10.3%)
Runs: 168 171 172 175 176 176 182 183 186 189 190 190 192 192 194 194 196 197 201 202 202 204 209 209 211 214 231 233 234 240 243

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/techievivek in version: 1.2.58-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/chiragsalian in version: 1.2.58-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 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.

4 participants