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

UI/clients list page progress #189

Merged
merged 11 commits into from
Mar 10, 2022
Merged

Conversation

ajinkyaa
Copy link
Contributor

@ajinkyaa ajinkyaa commented Mar 8, 2022

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 8, 2022 16:42 Inactive
@github-actions
Copy link

github-actions bot commented Mar 8, 2022

Current Code Coverage Percent of this PR:

71.09 %

Files having coverage below 100%

Impacted Files Coverage
/app/helpers/application_helper.rb 89.47 %
/app/helpers/team_helper.rb 85.71 %
/app/controllers/concerns/error_handler.rb 73.53 %
/app/controllers/time_tracking_controller.rb 36.36 %
/app/controllers/concerns/timesheet.rb 37.5 %
/app/controllers/workspaces_controller.rb 77.78 %
/app/controllers/root_controller.rb 42.86 %
/app/controllers/clients_controller.rb 94.44 %
/app/controllers/projects_controller.rb 42.86 %
/app/models/project.rb 90.91 %
/app/models/timesheet_entry.rb 73.08 %
/app/policies/client_policy.rb 75.0 %
/app/policies/timesheet_entry_policy.rb 50.0 %
/app/controllers/users/registrations_controller.rb 42.11 %
/app/controllers/users/passwords_controller.rb 41.67 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/users/invitations_controller.rb 64.29 %
/app/controllers/users/sessions_controller.rb 25.0 %
/app/controllers/companies/purge_logo_controller.rb 50.0 %
/app/controllers/internal_api/v1/workspaces_controller.rb 85.71 %
/app/controllers/internal_api/v1/clients_controller.rb 66.67 %
/app/controllers/internal_api/v1/timesheet_entry_controller.rb 42.86 %
/lib/authentication/google.rb 0.0 %

@supriya3105
Copy link
Contributor

supriya3105 commented Mar 9, 2022

@ajinkyaa Some feedback.
The dropdown should have three options i.e. week , month and year.
Also the title of the page is not completely visible.
The bar graph should only be visible to the owner and admin role. Currently it is being displayed for employee role too.

Copy link

@AkashKale AkashKale left a comment

Choose a reason for hiding this comment

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

  • Please update chart background color to #F5F7F9 (Light Gray 20)
  • Text color should be #1D1A31 (Dark Purple) everywhere and #4A485A(Dark Purple 80) for table header text
  • Table headers font size should be 12px with letter spacing of 2px
  • Chart text should be 14px with letter spacing of 2px
  • Table header background should be white
  • Number in the chart (below the bars) should have font color #777683 (Dark Purple 60), font size 14px and letter spacing of 2px
  • Can we remove the divider after the last row in the table?
  • Action buttons for each row should only appear on hover
  • Row should be highlighted on hover and should be clickable

@ajinkyaa
Copy link
Contributor Author

ajinkyaa commented Mar 9, 2022

  • Please update chart background color to #F5F7F9 (Light Gray 20)
  • Text color should be #1D1A31 (Dark Purple) everywhere and #4A485A(Dark Purple 80) for table header text
  • Table headers font size should be 12px with letter spacing of 2px
  • Chart text should be 14px with letter spacing of 2px
  • Table header background should be white
  • Number in the chart (below the bars) should have font color #777683 (Dark Purple 60), font size 14px and letter spacing of 2px
  • Can we remove the divider after the last row in the table?
  • Action buttons for each row should only appear on hover
  • Row should be highlighted on hover and should be clickable

@AkashKale
what happens once we click on the row?
what will be the hover color?

@AkashKale
Copy link

AkashKale commented Mar 9, 2022

  • Please update chart background color to #F5F7F9 (Light Gray 20)
  • Text color should be #1D1A31 (Dark Purple) everywhere and #4A485A(Dark Purple 80) for table header text
  • Table headers font size should be 12px with letter spacing of 2px
  • Chart text should be 14px with letter spacing of 2px
  • Table header background should be white
  • Number in the chart (below the bars) should have font color #777683 (Dark Purple 60), font size 14px and letter spacing of 2px
  • Can we remove the divider after the last row in the table?
  • Action buttons for each row should only appear on hover
  • Row should be highlighted on hover and should be clickable

@AkashKale what happens once we click on the row? what will be the hover color?

It is in the design - https://www.figma.com/file/4gdPDRPqGqQqPYsO3Qj8CG/Miru-App?node-id=674%3A2783
@ajinkyaa

When the user clicks on the client name it opens the client page

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 9, 2022 12:35 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 9, 2022 12:39 Inactive
@ajinkyaa
Copy link
Contributor Author

ajinkyaa commented Mar 9, 2022

  • Please update chart background color to #F5F7F9 (Light Gray 20)
  • Text color should be #1D1A31 (Dark Purple) everywhere and #4A485A(Dark Purple 80) for table header text
  • Table headers font size should be 12px with letter spacing of 2px
  • Chart text should be 14px with letter spacing of 2px
  • Table header background should be white
  • Number in the chart (below the bars) should have font color #777683 (Dark Purple 60), font size 14px and letter spacing of 2px
  • Can we remove the divider after the last row in the table?
  • Action buttons for each row should only appear on hover
  • Row should be highlighted on hover and should be clickable
  • add options in week select box
  • update flag for admin and owner for bar chart

@AkashKale @supriya Please check the todo list, its only few remaining. Please check other ones.

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 9, 2022 13:31 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 9, 2022 16:22 Inactive
@supriya3105
Copy link
Contributor

@ajinkyaa Looks ok to me. As discussed add a new card for the remaining changes.

  1. On Safari browser there is title related issue.
    @AkashKale please review the changes and @akhilgkrishnan Please review the code.

Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

Added some minor comments.

app/javascript/src/components/Clients/ClientBarGraph.tsx Outdated Show resolved Hide resolved
app/javascript/src/components/Clients/ClientBarGraph.tsx Outdated Show resolved Hide resolved
app/javascript/src/components/Clients/Client.tsx Outdated Show resolved Hide resolved
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 10, 2022 06:07 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 10, 2022 08:51 Inactive
@ajinkyaa
Copy link
Contributor Author

@AkashKale except font issues, other issues are resolved, please review
@akhilgkrishnan interface and all issues are solved, please review

Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

@ajinkyaa added some concerns

@@ -0,0 +1,27 @@
interface ClientArray {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this in a separate file. Can we use this in the same file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If some interfaces are similar in multiple components, we can export from interface file.

app/javascript/src/components/Clients/ChartBar.tsx Outdated Show resolved Hide resolved
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 10, 2022 13:40 Inactive
@vipulnsward vipulnsward temporarily deployed to miru-review-pr-189 March 10, 2022 13:41 Inactive
Copy link
Member

@akhilgkrishnan akhilgkrishnan left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@ajinkyaa ajinkyaa merged commit 4fcea3c into develop Mar 10, 2022
@ajinkyaa ajinkyaa deleted the UI/clients-list-page-progress branch March 10, 2022 14:23
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.

5 participants