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

fix(chat): removing dynamic scoping from chat section #5290

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

alexandraB99
Copy link
Contributor

Closes #4244

What does the PR do

Removed dynamic scoping from chat section

Affected areas

Chat section

@status-im-auto
Copy link
Member

status-im-auto commented Mar 31, 2022

Jenkins Builds

Click to see older builds (39)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 9c8c765 #1 2022-03-31 22:15:25 ~10 min linux 📦tgz
✔️ 9c8c765 #1 2022-03-31 22:23:52 ~18 min macos 📦dmg
✔️ 9c8c765 #1 2022-03-31 22:30:26 ~25 min windows 📦exe
✔️ 79b5068 #2 2022-04-06 22:10:56 ~8 min linux 📦tgz
✔️ 79b5068 #2 2022-04-06 22:16:36 ~14 min macos 📦dmg
✔️ 79b5068 #2 2022-04-06 22:20:35 ~18 min windows 📦exe
✔️ d5eeccc #3 2022-04-06 22:15:07 ~8 min macos 📦dmg
✔️ d5eeccc #3 2022-04-06 22:15:52 ~8 min linux 📦tgz
✔️ d5eeccc #3 2022-04-06 22:26:37 ~19 min windows 📦exe
✔️ 30ebaa5 #4 2022-04-06 22:16:18 ~8 min macos 📦dmg
✔️ 30ebaa5 #4 2022-04-06 22:17:19 ~9 min linux 📦tgz
✔️ 30ebaa5 #4 2022-04-06 22:38:21 ~30 min windows 📦exe
✔️ fecf197 #5 2022-05-19 14:13:42 ~9 min macos 📦dmg
✔️ fecf197 #5 2022-05-19 14:14:05 ~10 min linux 📦tgz
✔️ fecf197 #5 2022-05-19 14:26:40 ~22 min windows 📦exe
✔️ 3d43877 #6 2022-05-20 14:34:33 ~8 min macos 📦dmg
✔️ 3d43877 #6 2022-05-20 14:36:38 ~10 min linux 📦tgz
✔️ 3d43877 #6 2022-05-20 14:45:20 ~18 min windows 📦exe
✔️ 05f8e31 #7 2022-05-23 14:38:24 ~9 min macos 📦dmg
✔️ 05f8e31 #7 2022-05-23 14:42:59 ~14 min linux 📦tgz
✔️ 05f8e31 #7 2022-05-23 14:49:08 ~20 min windows 📦exe
✔️ 86dc079 #8 2022-06-08 14:47:57 ~9 min macos 📦dmg
✔️ 86dc079 #8 2022-06-08 14:49:07 ~10 min linux 📦tgz
✔️ 86dc079 #8 2022-06-08 15:00:49 ~22 min windows 📦exe
✔️ 05eef73 #9 2022-06-09 09:49:04 ~8 min macos 📦dmg
✔️ 05eef73 #9 2022-06-09 09:51:16 ~10 min linux 📦tgz
✔️ 05eef73 #9 2022-06-09 10:04:36 ~23 min windows 📦exe
✔️ 2ed9e54 #10 2022-06-17 11:21:04 ~8 min macos 📦dmg
✔️ 2ed9e54 #10 2022-06-17 11:22:45 ~10 min linux 📦tgz
✔️ 2ed9e54 #10 2022-06-17 11:34:26 ~21 min windows 📦exe
✔️ 60e85e9 #11 2022-06-24 17:27:30 ~8 min macos 📦dmg
✔️ 60e85e9 #11 2022-06-24 17:29:19 ~10 min linux 📦tgz
✔️ 60e85e9 #11 2022-06-24 17:53:28 ~34 min windows 📦exe
✔️ c29a407 #12 2022-06-27 15:50:50 ~9 min linux 📦tgz
✔️ c29a407 #12 2022-06-27 16:02:47 ~21 min windows 📦exe
✔️ c29a407 #12 2022-06-27 16:05:18 ~24 min macos 📦dmg
✔️ aa1b403 #13 2022-06-28 12:06:07 ~10 min linux 📦tgz
✔️ aa1b403 #13 2022-06-28 12:14:21 ~18 min macos 📦dmg
✔️ aa1b403 #13 2022-06-28 12:18:13 ~22 min windows 📦exe
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ eec1390 #14 2022-07-12 11:16:24 ~9 min macos 📦dmg
✔️ eec1390 #14 2022-07-12 11:17:07 ~9 min linux 📦tgz
✔️ eec1390 #14 2022-07-12 11:29:50 ~22 min windows 📦exe
✔️ 91c61c6 #15 2022-07-15 14:05:36 ~10 min linux 📦tgz
✔️ 91c61c6 #15 2022-07-15 14:10:06 ~14 min macos 📦dmg
✔️ 91c61c6 #15 2022-07-15 14:18:11 ~22 min windows 📦exe

@@ -15,6 +15,9 @@ QtObject {
property var mainModuleInst
property var privacyModuleInst
property var toastMessage
property var pinnedMessagesPopup
Copy link
Contributor

Choose a reason for hiding this comment

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

hey this doesnt seem consistent throughout now, for example we pass instance of the model to the sections that needs it and then call Global.openPopup(modalComponent) and now for these 3 its different.

Copy link
Contributor

@Khushboo-dev-cpp Khushboo-dev-cpp left a comment

Choose a reason for hiding this comment

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

LGTM. Only thing is about setting popup components in Global, its done in different ways currently. Need to think if we should come with an approach for Modals that is consistent.

@alexandraB99
Copy link
Contributor Author

LGTM. Only thing is about setting popup components in Global, its done in different ways currently. Need to think if we should come with an approach for Modals that is consistent.

@Khushboo-dev-cpp I totally agree and was thinking about the same. Some popups are wrapped inside Component and created only on demand, some others are instantiated as is and thus created on start up. I agree we need a clean/consistent solution for handling popups, ideally a Loader in AppMain and a Popups.qml singleton where all popups will be referenced as properties and then a function like Global.openPopup(Popups.profilePopup);...would something like this make sense?

@alexandraB99 alexandraB99 force-pushed the fix/issue-4244 branch 3 times, most recently from d5eeccc to 30ebaa5 Compare April 6, 2022 22:07
@alexandraB99 alexandraB99 requested a review from a team May 19, 2022 14:03
@@ -322,6 +331,7 @@ Item {
anchors.centerIn: parent
store: root.store
onClosed: {
Global.openPopup(communitiesPopupComponent)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be here?
Previously it was happening on CommunityDetailPopup.closed()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ui/app/AppLayouts/Chat/views/CommunitySettingsView.qml Outdated Show resolved Hide resolved
ui/app/AppLayouts/Chat/views/ChatContentView.qml Outdated Show resolved Hide resolved
@Hbouaz Hbouaz added the testing label May 23, 2022
@elina2015 elina2015 requested review from Hbouaz and removed request for a team May 23, 2022 23:18
@Hbouaz
Copy link

Hbouaz commented May 25, 2022

This will take some time to test as all 120+ Chat test cases need to be run, I am on it while also checking every detail on Figma designs

@Hbouaz
Copy link

Hbouaz commented Jun 8, 2022

will resume testing after a rebase, as agreed with @alexandraB99

@alexandraB99
Copy link
Contributor Author

@Hbouaz rebased

@Hbouaz
Copy link

Hbouaz commented Jul 14, 2022

@alexandraB99 quick check ok, if you merge I will check more on master as part of regression testing run

@alexandraB99 alexandraB99 merged commit 2bd6859 into master Jul 15, 2022
@alexandraB99 alexandraB99 deleted the fix/issue-4244 branch July 15, 2022 14:00
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.

[refactor] remove dynamic scoping in chat section
6 participants