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

feat: MMPD-1528 - View recents and favorites in browser URL modal #12938

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

ziad-saab
Copy link
Contributor

@ziad-saab ziad-saab commented Jan 10, 2025

Description

https://consensyssoftware.atlassian.net/browse/MMPD-1528

Change the behaviour of the browser url autocomplete:

  • add scrolling
  • when nothing is typed, show recents (max 5) and favorites
  • when user starts typing, auto complete from dapp list + recents + favorites
  • adjust styling to match new figma styles

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/MMPD-1528

Manual testing steps

  1. Go to the in-app browser
  2. Press the URL bar
  3. Observe that the currently typed URL is available as a result, just like in the current version
  4. Clear the input and observe that you see your (up to) 5 latest recents, + favorites, and that you can scroll
  5. Start typing, and observe that you get up to 3 lists: Sites, Recents, and Favorites. The elements shown are matched from what you're typing

Screenshots/Recordings

Before

After

image image image image

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

- when nothing is typed, show recents (max 5) and favorites
- when user starts typing, show results from the dapp list
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@ziad-saab ziad-saab added needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-portfolio labels Jan 10, 2025
@ziad-saab ziad-saab marked this pull request as ready for review January 13, 2025 20:30
@ziad-saab ziad-saab requested a review from a team as a code owner January 13, 2025 20:30
@ziad-saab ziad-saab added No E2E Smoke Needed If the PR does not need E2E smoke test run QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jan 13, 2025
@ziad-saab ziad-saab marked this pull request as draft January 13, 2025 20:56
@ziad-saab ziad-saab marked this pull request as ready for review January 13, 2025 21:49
@ziad-saab ziad-saab added Run Smoke E2E Triggers smoke e2e on Bitrise and removed No E2E Smoke Needed If the PR does not need E2E smoke test run labels Jan 13, 2025
Copy link
Contributor

github-actions bot commented Jan 13, 2025

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 4af548d
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/258a6bb8-0746-4cdf-aa7b-051017c7d197

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@ziad-saab ziad-saab removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 14, 2025
@ziad-saab ziad-saab added the Run Smoke E2E Triggers smoke e2e on Bitrise label Jan 14, 2025
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: bb0fae8
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/be044e8e-a84b-4490-8fdd-419be90fa4ae

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@ziad-saab ziad-saab added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 14, 2025
Copy link
Contributor

github-actions bot commented Jan 14, 2025

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 01270c6
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/82b4defb-617b-46a3-8d76-0fb8bbc59ffc

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@ziad-saab ziad-saab enabled auto-merge January 14, 2025 21:06
@ziad-saab ziad-saab added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 15, 2025
Copy link
Contributor

github-actions bot commented Jan 15, 2025

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: f0b234e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/78b61287-17b4-4af6-a54a-540300ffacb5

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left some comments

app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
category: {
fontSize: 16,
color: colors.text.default,
...fontStyles.bold,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use typography from theme based on what font style Figma is showing. For example, reference Input.styles.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Cal-L need more details on this one please

app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
app/components/UI/UrlAutocomplete/index.js Outdated Show resolved Hide resolved
borderRadius: 8,
borderWidth: 1,
borderColor: colors.border.default,
height: Device.isAndroid() ? 50 : 40,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to have different heights for the platforms or can we just use one value?

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Heads up that the many use of array methods in the UrlAutocomplete may potentially cause performance issues. It seems that there could be a more optimal way to store the values to reduce the amount of transformations. Happy to pair on this

@ziad-saab ziad-saab requested a review from Cal-L January 15, 2025 20:36
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left a comment

@@ -268,24 +159,18 @@ export class UrlAutocomplete extends PureComponent {
);
}

const categoriesWithResults = ORDERED_CATEGORIES.filter(category => resultsByCategory[category]?.length > 0);

return (
<ScrollView style={styles.wrapper} contentContainerStyle={styles.contentContainer}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be updated to be a Flatlist instead of a Scrollview since it abstracts the need to map over the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA Passed A successful QA run through has been done Run Smoke E2E Triggers smoke e2e on Bitrise team-portfolio
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

2 participants