fix: Optimize ContactsView & MembersTabPanel settings pages #16979
Conversation
Jenkins BuildsClick to see older builds (96)
|
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
2 times, most recently
from
d0e8549 to
dd6550f
Compare
caybro
changed the title
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
3 times, most recently
from
285991f to
03f9af4
Compare
caybro
requested review from
Khushboo-dev-cpp,
virginiabalducci,
jrainville and
anastasiyaig
and removed request for
a team,
virginiabalducci and
Khushboo-dev-cpp
|
I did some quick tests on the Status Community. The performance looks fine. The only issue I found is that the Banned list is empty I tried master and it works fine, so there is a small broken thing there |
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
from
03f9af4 to
55747bf
Compare
Ah great, good spot! Wrong enum, fixed :) |
|
@caybro i will fix the tests, looking into that now. Just one question: was the second checkbox changed to be not a checkbox on purpose?
|
Oh definitely not and I have no idea what happened here, will check |
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
from
55747bf to
2052cd3
Compare
And fixed :) |
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
from
2052cd3 to
77975e9
Compare
anastasiyaig
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
2 times, most recently
from
6c476aa to
3393297
Compare
|
A small bug I found is that the checkboxes don't seem correct in the Edit community panel. There is a weird "selected" effect on the first two letters and the checkboxes don't show (on the right maybe)
The button to save the edit doesn't appear either. The member lists seem to work fine. but I couldn't test the pending members because of the above bugs |
Ah thanks, will have a look tomorrow! |
Looks a similar prob to what Nastya found above; should be an easy fix |
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
2 times, most recently
from
088fc8f to
f0809e6
Compare
|
@jrainville addressed your feedback, and fixed the issue with checkboxes; pls have a second look 🙏 |
|
On the tester side, everything works as expected, except a small bug with rejecting a request, but I tested it on master and the issue happens there too, so I'll open a new issue. Anyway, tested ✔️ |
- fixup margins and padding according to latest Figma designs - make a difference between a disabled and inactive tab by using opacity - provide smooth color transitions - add a dedicated StoryBook page
- fix a bug where the Switch would start animating if it'd been checked on creation - add the same property `leftSide` to StatusSwitch (just like StatusCheckBox), and use `LayoutMirroring` to perform the visual inversion - fixup margins and padding, removing hardcoded values, according to latest Figma designs - make a difference between a disabled and inactive button by using opacity - provide smooth color transitions - add dedicated StoryBook pages
- removed nested ListViews inside StackLayouts, in order to reduce the memory footprint and improve performance, and also to be able to better manage the scrolling - no more unrolled multiple listviews, which again hurt the performance; now the views instantiate the delegates dynamically on the fly - the tab bar and the search fields now stick to the top of the page, with the users list view scrolling independently - both views now uniformly use the common `ContactListItemDelegate` - the received/sent CRs are now combined into one `pendingContacts` model - factored out common search/filter criteria into a new, separate SFPM `UserFilterContainer` component - fix an issue where StatusContactVerificationIcons wasn't properly displaying the "blocked" state/icon - fix documentation comments, removed relative imports, and updated some Fixes #16612 Fixes #16958
caybro
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
from
f0809e6 to
9b0cca1
Compare
There was a problem hiding this comment.
Great job, finally fixing something that hurt my eyes every time I looked at it ;)
There is one thing that could be changed to improve behavior and simplify code, details in the comments (mainly #16979 (comment)).
| interactive: false | ||
| model: SortFilterProxyModel { | ||
| id: filteredModel | ||
| objectName: "ContactListPanel_ListView" |
There was a problem hiding this comment.
Detail: it's good practice to not set the objectName at the top level. Please set in a place where ContactListPanel is instantiated.
micieslak
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
from
9b0cca1 to
bff2a49
Compare
micieslak
force-pushed
the
16612-settings-optimize-contactsview-settings-page
branch
from
bff2a49 to
b78e369
Compare
|
I added additional commit addressing my review comments. @caybro @jrainville please re-test. It brings several simplifications making the code shorter and changes the behavior I mentioned earlier (tabs are stateful now). It may be still needed to adjust e2e test. |
|
App still works as expected and I can confirm that the list keeps the state when switching from one tab to the other! |
Tip
Best review commit by commit; the first 2 contain UI components updates. Also, a lot of the new stuff is Storybook pages :)
What does the PR do
ContactListItemDelegatependingContactsmodelUserFilterContainercomponentFixes #16612
Fixes #16958
Affected areas
Settings/Contacts; Community/Settings/Members
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)
Contacts:
Community members:
