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(@desktop): enable the hover effects by default #6335

Merged
merged 2 commits into from
Jul 5, 2022
Merged

fix(@desktop): enable the hover effects by default #6335

merged 2 commits into from
Jul 5, 2022

Conversation

caybro
Copy link
Member

@caybro caybro commented Jul 4, 2022

Close #6254

  • since Qt 5.xy, hover is not enabled by default for QQC2, so enable it
    unconditionally as we are a desktop app anyway
  • this fixes several hover effects being broken, mostly for builtin
    components like MenuItem and some buttons (eg. the leftmost NavBar)
    where we haven't enabled those with hoverEnabled: true explicitely
  • additionally, provide a tooltip for the default "chat" section which
    doesn't seem to supply its own (unlike the other sections)

What does the PR do

Fixes mouse over (hover) effects being (mostly) broken

Affected areas

All areas of status-desktop

Screenshot of functionality

Snímek obrazovky z 2022-07-04 12-41-28
Snímek obrazovky z 2022-07-04 12-42-10

@status-im-auto
Copy link
Member

status-im-auto commented Jul 4, 2022 •
edited
Loading

Jenkins Builds

Click to see older builds (4)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
⁉️ 1b11526 #1 2022-07-04 11:39:34 ~2 min linux-cpp 📄log
✔️ 1b11526 #1 2022-07-04 11:45:01 ~8 min macos 📦dmg
✔️ 1b11526 #1 2022-07-04 11:46:53 ~10 min linux 📦tgz
✔️ 1b11526 #1 2022-07-04 11:59:20 ~22 min windows 📦exe
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
⁉️ de34de8 #2 2022-07-04 12:08:25 ~2 min linux-cpp 📄log
✔️ de34de8 #2 2022-07-04 12:13:42 ~7 min macos 📦dmg
✔️ de34de8 #2 2022-07-04 12:15:49 ~10 min linux 📦tgz
✔️ de34de8 #2 2022-07-04 12:27:22 ~21 min windows 📦exe
⁉️ 064527a #3 2022-07-04 15:43:59 ~2 min linux-cpp 📄log
✔️ 064527a #3 2022-07-04 15:49:14 ~7 min macos 📦dmg
✔️ 064527a #3 2022-07-04 15:51:16 ~9 min linux 📦tgz
✔️ 064527a #3 2022-07-04 16:05:20 ~23 min windows 📦exe

igor-sirotin
igor-sirotin requested changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@igor-sirotin igor-sirotin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is minor, though has to be fixed 🙂

ui/app/mainui/AppMain.qml Outdated Show resolved Hide resolved
0x-r4bbit
0x-r4bbit requested changes Jul 4, 2022
View reviewed changes
Copy link
Member

@0x-r4bbit 0x-r4bbit left a comment

Choose a reason for hiding this comment

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

The actual fix is of this commit is awesome!
Didn't know this existed. Setting hoverEnabled explicitly is quite annoying.

Bonus points if the fix for the tooltip is not in the commit that fixes the hover (no biggie if you don't feel like it)

ui/app/mainui/AppMain.qml Outdated Show resolved Hide resolved
osmaczko
osmaczko approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@osmaczko osmaczko left a comment

Choose a reason for hiding this comment

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

👍

@caybro
fix(@desktop): enable the hover effects by default
d0a26a3 
Close #6254

- since Qt 5.xy, hover is not enabled by default for QQC2, so enable it
unconditionally as we are a desktop app anyway
- this fixes several hover effects being broken, mostly for builtin
components like MenuItem and some buttons (eg. the leftmost NavBar)
where we haven't enabled those with `hoverEnabled: true` explicitely
@igor-sirotin igor-sirotin self-requested a review July 4, 2022 13:06
@caybro caybro requested a review from 0x-r4bbit July 4, 2022 14:42
@caybro caybro mentioned this pull request Jul 4, 2022
Merged
10 tasks
jrainville
jrainville requested changes Jul 4, 2022
View reviewed changes
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Nice fixes. The fix for the tooltip should be done in Nim however. I don't blame you for not knowing, it's not clear

ui/app/mainui/AppMain.qml Outdated Show resolved Hide resolved
@caybro
fix(nim/module): fix default tooltip for Chat section
064527a 
provide the default "Chat" name for the personal chat
@caybro caybro requested a review from jrainville July 4, 2022 15:41
jrainville
jrainville approved these changes Jul 4, 2022
View reviewed changes
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Good job

@caybro caybro requested review from 0x-r4bbit and removed request for 0x-r4bbit and alexandraB99 July 5, 2022 07:01
0x-r4bbit
0x-r4bbit approved these changes Jul 5, 2022
View reviewed changes
Copy link
Member

@0x-r4bbit 0x-r4bbit 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

@caybro
Copy link
Member Author

caybro commented Jul 5, 2022

@noelia-santos can you please give this one a test too please? TIA

@osmaczko osmaczko requested review from a team and Hbouaz and removed request for a team July 5, 2022 08:27
@Hbouaz Hbouaz added the testing label Jul 5, 2022
Hbouaz
Hbouaz approved these changes Jul 5, 2022
View reviewed changes
Copy link

@Hbouaz Hbouaz left a comment

Choose a reason for hiding this comment

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

nice one

@Hbouaz Hbouaz removed the testing label Jul 5, 2022
@Hbouaz Hbouaz added the tested label Jul 5, 2022
@caybro caybro merged commit 6c15509 into master Jul 5, 2022
@caybro caybro deleted the fix/hover-effects branch July 5, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osmaczko osmaczko osmaczko approved these changes

@0x-r4bbit 0x-r4bbit 0x-r4bbit approved these changes

@alaibe alaibe alaibe approved these changes

@jrainville jrainville jrainville approved these changes

@igor-sirotin igor-sirotin igor-sirotin approved these changes

@Hbouaz Hbouaz Hbouaz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Status Desktop/Mobile Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

There is no hint for the Chat tab
8 participants
@caybro @status-im-auto @0x-r4bbit @alaibe @jrainville @igor-sirotin @Hbouaz @osmaczko