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.

Sign up for GitHub

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

Jump to bottom

Fix/issue 8118 #8150

Merged
merged 4 commits into from
Nov 8, 2022
Merged

Fix/issue 8118 #8150

merged 4 commits into from
Nov 8, 2022

Conversation

alexjba
Copy link
Contributor

@alexjba alexjba commented Nov 7, 2022

There are two main changes in the model -> view relationship:

  1. The model coming from the store becomes the only source of truth and the UI will only change when the model changes. This means that when another UI component from settings menu wants to change some setting it needs to update the model, not UI components directly.
  2. When the store will provide a new model we will update only rows that are different in the current model.
  • Fix Display message links preview switch that was not being disabled if the last disabled item was the Image Unfurling

What does the PR do

Fixing bug #8118

Affected areas

Messaging settings view
Display message links preview in the chat
Save messaging settings to perssistancy

StatusQ checklist

  • add documentation if necessary (new component, new feature)
  • update sandbox app
    • in case of new component, add new component page
    • in case of new features, add variation to existing component page
    • nice to have: add it to the demo application as well
  • test changes in both light and dark theme?

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it
Screen.Recording.2022-11-07.at.17.21.22.mov

Cool Spaceship Picture

@alexjba
Display message preview settings - fix model -> view relationship
c68ec37 
There are two main changes in the model -> view relationship:
1. The model coming from the store becomes the only source of truth and the UI will only change when the model changes. This means that when another UI component from settings menu wants to change some setting it needs to update the model, not UI components directly.
2. When the store will provide a new model we will update only rows that are different in the current model.
@alexjba
Fix showMessageLinksSwitch bindings
d8e095d 
Property bindings were added for showMessageLinksSwitch in order for it to change the checked state whenever previewableSites model has any item checked or displayChatImages setting is true.
@status-im-auto
Copy link
Member

status-im-auto commented Nov 7, 2022 •
edited
Loading

Jenkins Builds

Click to see older builds (4)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ d8e095d #1 2022-11-07 15:34:08 ~5 min linux-cpp 📦pkg
✔️ d8e095d #1 2022-11-07 15:41:41 ~13 min linux 📦tgz
✔️ d8e095d #1 2022-11-07 15:45:37 ~17 min macos 📦dmg
⁉️ d8e095d #1 2022-11-07 15:55:10 ~26 min e2e 📄log
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ dfbae12 #2 2022-11-07 15:52:40 ~5 min linux-cpp 📦pkg
✔️ dfbae12 #2 2022-11-07 15:58:42 ~11 min linux 📦tgz
✔️ dfbae12 #2 2022-11-07 16:02:14 ~15 min macos 📦dmg
✔️ dfbae12 #2 2022-11-07 16:12:16 ~25 min windows 📦exe
✔️ 73b0119 #3 2022-11-07 16:31:38 ~5 min linux-cpp 📦pkg
✔️ 73b0119 #3 2022-11-07 16:36:12 ~10 min macos 📦dmg
✔️ 73b0119 #3 2022-11-07 16:38:39 ~12 min linux 📦tgz
✔️ 73b0119 #3 2022-11-07 16:49:12 ~23 min windows 📦exe
⁉️ 73b0119 #2 2022-11-07 17:19:46 ~26 min e2e 📄log

caybro
caybro requested changes Nov 7, 2022
View reviewed changes
Copy link
Member

@caybro caybro left a comment

Choose a reason for hiding this comment

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

Overall looks good, just some minor improvements needed imho

ui/app/AppLayouts/Profile/views/MessagingView.qml Outdated Show resolved Hide resolved
ui/app/AppLayouts/Profile/views/MessagingView.qml Outdated Show resolved Hide resolved
ui/app/AppLayouts/Profile/views/MessagingView.qml Outdated Show resolved Hide resolved
ui/app/AppLayouts/Profile/views/MessagingView.qml Show resolved Hide resolved
ui/app/AppLayouts/Profile/views/MessagingView.qml Show resolved Hide resolved
alexjba and others added 2 commits November 7, 2022 17:46
@alexjba @caybro
Update ui/app/AppLayouts/Profile/views/MessagingView.qml
dfbae12 
Simplify if statement

Co-authored-by: Lukáš Tinkl <lukast@status.im>
@alexjba
Update MessagingView.qml
73b0119 
Group all common functions
caybro
caybro approved these changes Nov 7, 2022
View reviewed changes
Copy link
Member

@caybro caybro 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; although in general there's too much JS code :)

stefandunca
stefandunca approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@stefandunca stefandunca left a comment

Choose a reason for hiding this comment

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

Great job and courage, I remember it was messy in there.

@alexjba alexjba merged commit d23cce9 into master Nov 8, 2022
@alexjba alexjba deleted the fix/issue-8118 branch November 8, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caybro caybro caybro approved these changes

@stefandunca stefandunca stefandunca approved these changes

@micieslak micieslak Awaiting requested review from micieslak

@alexandraB99 alexandraB99 Awaiting requested review from alexandraB99

@noeliaSD noeliaSD Awaiting requested review from noeliaSD

@saledjenic saledjenic Awaiting requested review from saledjenic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexjba @status-im-auto @caybro @stefandunca