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

Wrap note text on reports page #203

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Wrap note text on reports page #203

merged 1 commit into from
Mar 17, 2022

Conversation

shalapatil
Copy link
Contributor

@shalapatil shalapatil commented Mar 15, 2022

Notion card

Summary

https://www.notion.so/saeloun/Text-wrapping-is-required-on-notes-field-on-time-entry-report-5d312940b2644b879dd076d7496b8fd2
Fixed the wrapping of note on reports page

Preview

Before
Screenshot 2022-03-15 at 2 56 34 PM

after
Screenshot 2022-03-15 at 2 55 34 PM

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Checklist:

  • I have manually tested all workflows
  • I have performed a self-review of my own code
  • I have added automated tests for my code

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-203 March 15, 2022 09:18 Inactive
@github-actions
Copy link

github-actions bot commented Mar 15, 2022

Current Code Coverage Percent of this PR:

78.3 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/concerns/error_handler.rb 73.53 %
/app/controllers/projects_controller.rb 42.86 %
/app/controllers/concerns/timesheet.rb 75.0 %
/app/controllers/workspaces_controller.rb 77.78 %
/app/controllers/clients_controller.rb 94.44 %
/app/models/company.rb 93.75 %
/app/models/timesheet_entry.rb 76.92 %
/app/models/project.rb 93.33 %
/app/policies/timesheet_entry_policy.rb 50.0 %
/app/policies/client_policy.rb 80.0 %
/app/controllers/users/passwords_controller.rb 41.67 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/users/sessions_controller.rb 25.0 %
/app/controllers/users/registrations_controller.rb 40.0 %
/app/controllers/internal_api/v1/reports_controller.rb 88.89 %
/app/controllers/internal_api/v1/timesheet_entry_controller.rb 42.86 %
/app/controllers/internal_api/v1/workspaces_controller.rb 85.71 %
/app/controllers/internal_api/v1/clients_controller.rb 80.77 %
/lib/authentication/google.rb 0.0 %

@supriya3105
Copy link
Contributor

@shalapatil got this on testing it

Credentials used for testing
username : vipul@example.com
password : password

Screenshot 2022-03-15 at 2 57 56 PM

Copy link
Contributor

@rohitjoshixyz rohitjoshixyz left a comment

Choose a reason for hiding this comment

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

LGTM for now, should we consider truncating the note if it's very long, say more than 2000 characters, at least for the index page? cc @keshavbiswa @akhilgkrishnan

Copy link
Contributor

@apoorv-mishra apoorv-mishra left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@supriya3105
Copy link
Contributor

@shalapatil Updated the screenshot of the test I ran for this fix. I can still see that the text is not wrapped.

@keshavbiswa
Copy link
Contributor

LGTM for now, should we consider truncating the note if it's very long, say more than 2000 characters, at least for the index page? cc @keshavbiswa @akhilgkrishnan

Yes I think truncating long texts would be a good idea.

@rohitjoshixyz
Copy link
Contributor

Can you try mobile view too, @shalapatil

@supriya3105
Copy link
Contributor

@shalapatil Please resolve the conflicts on this PR
@keshavbiswa Please review

Copy link
Contributor

@keshavbiswa keshavbiswa left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-203 March 17, 2022 07:41 Inactive
@shalapatil
Copy link
Contributor Author

@shalapatil Updated the screenshot of the test I ran for this fix. I can still see that the text is not wrapped.

@supriya3105 This are the latest screenshots, working fine
Screenshot 2022-03-17 at 1 04 07 PM
Screenshot 2022-03-17 at 1 13 46 PM

@shalapatil
Copy link
Contributor Author

I will work on truncate in separate commit

@supriya3105 supriya3105 self-requested a review March 17, 2022 08:42
@rohitjoshixyz rohitjoshixyz merged commit e7516e4 into develop Mar 17, 2022
@akhilgkrishnan akhilgkrishnan deleted the wrap-report-notes branch April 5, 2022 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants