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: pinned apps on top of all apps + rm list view switch #2699

Merged
merged 7 commits into from
Oct 30, 2023
Merged

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Oct 27, 2023

What it solves

Removes the "Bookmarked apps" tab and instead shows pinned app on top of the main list.

Screenshot 2023-10-27 at 12 40 24

Also we're removing the list/grid layout switch because of a very low usage. Last 60 days:
Screenshot 2023-10-27 at 13 15 22


Design: https://www.figma.com/file/ptTs6lDBeUuLNySroJ5PiF/Web-Master-File?type=design&node-id=2102-55784&mode=design&t=9f1b7qvhnoHSaIFl-0

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Branch preview

✅ Deploy successful!

https://pinned--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.27% (-0.06% 🔻)
9404/12494
🔴 Branches
49.47% (-0.03% 🔻)
1923/3887
🔴 Functions
58.2% (-0.02% 🔻)
1420/2440
🟡 Lines
76.9% (-0.05% 🔻)
8511/11067
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / index.tsx
96% (-0.77% 🔻)
90.91% (-2.42% 🔻)
100% 100%

Test suite run success

1044 tests passing in 142 suites.

Report generated by 🧪jest coverage report action from e15efc1

@katspaugh katspaugh changed the title Feat: pinned apps on top of all apps Feat: pinned apps on top of all apps + rm list view switch Oct 27, 2023
@katspaugh katspaugh marked this pull request as ready for review October 27, 2023 12:25
@katspaugh katspaugh requested a review from usame-algan October 27, 2023 12:25
Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

<SafeAppsSDKLink />
// Redirect to /apps
useEffect(() => {
router.push({ pathname: AppRoutes.apps.index, query: { safe: router.query.safe } })
Copy link
Member

Choose a reason for hiding this comment

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

Should we use router.replace instead so the user doesn't have to go back twice?

@francovenica
Copy link
Contributor

Looks good to me.

The Bookmark Tab is gone
The "My pinned apps" subsection shows when at least 1 pinned app is present
The Grid/list view buttons were removed

Note:
The filters "Category" and "Optimized with" are hiding the pinned apps. Those filters were not in the bookmarked apps so it's not an issue, but I don't see a reason why they cannot also filter the new "My pinned apps" new subsection. So I'll create a new ticket for it

@katspaugh
Copy link
Member Author

@francovenica they filter all apps together, pinned and unpinned mixed in one filtered list.

@katspaugh katspaugh merged commit 61f0568 into dev Oct 30, 2023
8 of 9 checks passed
@katspaugh katspaugh deleted the pinned branch October 30, 2023 13:50
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants