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: Added a close button to the tooltip in Onboarding #4619

Merged
merged 8 commits into from
Sep 6, 2023
Merged

fix: Added a close button to the tooltip in Onboarding #4619

merged 8 commits into from
Sep 6, 2023

Conversation

sm-sayedi
Copy link
Contributor

What

  • A close button is added to the tooltip top-right corner in the onboarding flow.
  • Now the user can tap on the close icon to close the tooltip instead of tapping on all of the tooltip.

Screenshots

Before After

Fixes bug(s)

Part of

@sm-sayedi sm-sayedi requested a review from a team as a code owner September 3, 2023 09:16
@github-actions github-actions bot added the 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… label Sep 3, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2023 •
edited
Loading

Codecov Report

Merging #4619 (21e194f) into develop (e421da3) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #4619      +/-   ##
===========================================
- Coverage    10.13%   10.13%   -0.01%     
===========================================
  Files          299      299              
  Lines        15692    15695       +3     
===========================================
  Hits          1591     1591              
- Misses       14101    14104       +3
Files Changed Coverage Δ
...ages/onboarding/knowledge_panel_page_template.dart 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

monsieurtanuki
monsieurtanuki reviewed Sep 5, 2023
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @sm-sayedi!
Not too bad, but please have a look at my comment.

packages/smooth_app/lib/pages/onboarding/knowledge_panel_page_template.dart Outdated
@@ -149,30 +149,44 @@ class _KnowledgePanelPageTemplateState
);

List<Widget> _buildHintPopup() {
final Widget hintPopup = InkWell(
final Widget hintPopup = Card(
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need the whole tooltip to be closable by click. The cross icon is just to tell the user "btw you can close this tooltip" (but you can click anywhere).
Besides if you want an icon to be clickable, you're supposed to make it "big enough" (something like 48) and it would be too big for the tooltip.

@sm-sayedi
Copy link
Contributor Author

Hi @monsieurtanuki,

We still need the whole tooltip to be closable by click. The cross icon is just to tell the user "btw you can close this tooltip" (but you can click anywhere). Besides if you want an icon to be clickable, you're supposed to make it "big enough" (something like 48) and it would be too big for the tooltip.

I have updated my code according to your comment.

monsieurtanuki
monsieurtanuki reviewed Sep 5, 2023
View reviewed changes
packages/smooth_app/lib/pages/onboarding/knowledge_panel_page_template.dart Outdated
Comment on lines 175 to 180
InkWell(
onTap: () {
setState(() {
_isHintDismissed = true;
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need that; there's already an InkWell on top, isn't there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monsieurtanuki You're right! I have updated the code.

monsieurtanuki
monsieurtanuki approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @sm-sayedi!
Looks good to me - except perhaps the color of the icon (I don't know how this is supposed to look in dark mode). Anyway I'm about to fix it.

@monsieurtanuki monsieurtanuki merged commit b530a37 into openfoodfacts:develop Sep 6, 2023
@github-actions github-actions bot mentioned this pull request Sep 6, 2023
Merged
@sm-sayedi
Copy link
Contributor Author

@monsieurtanuki Thank you! I think it looks the same in the dark mode too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monsieurtanuki monsieurtanuki monsieurtanuki approved these changes

Assignees

@sm-sayedi sm-sayedi

Labels
🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score…
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve the tooltip by adding a Close icon
3 participants
@sm-sayedi @codecov-commenter @monsieurtanuki