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/project list page updations #281

Merged
merged 5 commits into from
Apr 18, 2022
Merged

Conversation

Shruti-Apte
Copy link
Contributor

Notion card

https://www.notion.so/saeloun/Remaining-work-on-Project-list-page-refactoring-22ad94893d14411d8e565160c38a40c8

Summary

API integration for project list page

Preview

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Checklist:

  • I have performed a self-review of my own code

@github-actions
Copy link

Current Code Coverage Percent of this PR:

97.79 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/clients_controller.rb 94.44 %
/app/models/project.rb 90.32 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/internal_api/v1/timesheet_entry/bulk_action_controller.rb 50.0 %


useEffect(() => {
const getClientList = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why write a function for this?
Also code should be
await projectApi.get() .then(res => {// set state}) .catch(e => {//error handling}

Copy link
Contributor

@shivamsinghchahar shivamsinghchahar Apr 18, 2022

Choose a reason for hiding this comment

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

@ajinkyaa Why use .then and .catch with await?

We should not be using .then and .catch with async/await. It defeats the whole purpose of having async/await in the first place.

We should write it like this:

const fetchClients = async () => {
  try {
    const data = await projectApi.get();
    // state updates
  } catch {
    // handle errors
  }
}

cc// @Shruti-Apte

}).then(() => {
setEditProjectData("");
setShowProjectModal(false);
window.location.reload();
Copy link
Contributor

Choose a reason for hiding this comment

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

why reload the window?

@vipulnsward vipulnsward merged commit 0ca699c into develop Apr 18, 2022
@vipulnsward vipulnsward deleted the UI/project_list_page_updations branch April 18, 2022 11:41
@vipulnsward
Copy link
Contributor

Please send another PR

@Shruti-Apte
Copy link
Contributor Author

Please send another PR

Okay! with loom video you mean?

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.

4 participants