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:Updating UI #3574

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix:Updating UI #3574

wants to merge 3 commits into from

Conversation

SUNIL-RAGHU
Copy link

Added vertical padding because the text was appearing too close to the top.

Before:
Before Screenshot

After:
After Screenshot

In this commit, I adjusted the content padding to include both horizontal and vertical padding in order to improve the visual appearance. The text was previously too close to the top, which could lead to readability issues. This change enhances the overall user experience by providing better spacing.

  • Modified contentPadding in the relevant widget.
  • Included 'horizontal' and 'vertical' parameters with appropriate values.

Screenshots have been included for visual comparison between the original and modified layouts.

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2023

CLA assistant check
All committers have signed the CLA.

@Xazin
Copy link
Collaborator

Xazin commented Oct 2, 2023

To me it seems it's still preferential to the top, can we make it centered?

@SUNIL-RAGHU
Copy link
Author

yes! will change it then

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (890f52a) 69.61% compared to head (4c2e604) 67.71%.
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3574      +/-   ##
==========================================
- Coverage   69.61%   67.71%   -1.90%     
==========================================
  Files         502      529      +27     
  Lines       23253    24704    +1451     
==========================================
+ Hits        16187    16729     +542     
- Misses       7066     7975     +909     
Flag Coverage Δ
appflowy_flutter_integrateion_test 65.70% <ø> (-1.79%) ⬇️
appflowy_flutter_unit_test 12.17% <ø> (-0.50%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ns/openai/widgets/auto_completion_node_widget.dart 2.00% <ø> (ø)

... and 96 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SUNIL-RAGHU
Copy link
Author

Check now!
Screenshot 2023-10-02 at 10 10 47 PM

@richardshiue
Copy link
Collaborator

Adding vertical padding, that I can get behind. But why alter the horizontal padding as well?

Looking at the code, it seems like the max line argument is 3 here. I suggest instead that you also remove the constraints altogether in FlowyTextField and set isDense to true.

@SUNIL-RAGHU
Copy link
Author

Before Modification:
Screenshot 2023-10-03 at 11 03 10 PM

After Modification:
Screenshot 2023-10-03 at 11 12 51 PM

Changes Made:

Updated the TextField widget behavior to allow for the input of multiple lines of text.
Implemented dynamic expansion of the widget to ensure clear visibility of entered text, even when more than one line is used.

Benefits:

Enhances user experience by accommodating longer or multiline inputs without sacrificing visibility or usability.
Provides a more flexible and intuitive text input experience for users.

@richardshiue
Copy link
Collaborator

I don't know where the 13 symmetrical vertical padding comes from but looks good to me

@SUNIL-RAGHU
Copy link
Author

Added vertical padding because the text was appearing too close to the top.

Before: Before Screenshot

@Xazin Xazin self-requested a review February 16, 2024 17:52
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.

None yet

4 participants