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

[OP#41124] Display loading icon while fetching workpackages #55

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

SwikritiT
Copy link
Collaborator

@SwikritiT SwikritiT commented Feb 21, 2022

part of OP#41124: This PR adds a loading icon when fetching work packages

It looks something like this:
loading

Comment on lines 58 to 60
<template #noResult>
{{ noResultMessages }}
</template>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@eneiluj The messages in this slot are not being displayed in the UI. The default one No result is displayed but if I define a custom message it's not displayed. Am I doing something wrong or that slot can't be used?

Copy link
Contributor

Choose a reason for hiding this comment

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

the #noResult slot is actually from vue-multiselect library and is already implemented inside the Multiselect.vue so I think, we cannot further use it as a slot here.

@SwikritiT SwikritiT self-assigned this Feb 21, 2022
@@ -177,10 +191,11 @@ export default {
async getWorkPackageColorAttributes(path, id) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this whole function seems not to be covered well with tests, maybe this PR is a good option to add more

tests/jest/components/tab/SearchInput.spec.js Outdated Show resolved Hide resolved
src/components/tab/SearchInput.vue Outdated Show resolved Hide resolved
Comment on lines +187 to +189
data: {
color: 'red',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

data is also redundant here imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it gives me the error TypeError: Cannot read property 'color' of undefined, if I don't define data

Copy link
Collaborator

Choose a reason for hiding this comment

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

can be done in an other PR, but I would suggest to improve getWorkPackageColorAttributes
it checks if (response.status === 200) { and the assumes that the response has a color attribute, but what if not?
Should be something like if (response.status === 200 && response.data.color !== undefined) {

Signed-off-by: Swikriti Tripathi <swikriti808@gmail.com>
@github-actions
Copy link

JS Code Coverage

Coverage after merging loading-icon into master will be
42.15%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/components
   AdminSettings.vue0%100%0%0%101, 48–54, 66, 82–84, 88, 95–96, 98
   OAuthConnectButton.vue100%100%100%100%
   PersonalSettings.vue33.33%26.32%27.27%38.89%102–103, 103, 103, 106–107, 107, 107, 110–111, 111, 111, 114, 117–118, 120–121, 121–123, 123, 123, 123–124, 129–130, 130, 130, 135, 141, 89–92, 98–99
src/components/settings
   SettingsTitle.vue0%100%0%0%14
src/components/tab
   EmptyContent.vue100%100%100%100%
   SearchInput.vue69.15%45.45%90.91%73.77%100–101, 101, 101, 101–103, 111–113, 116–119, 122–123, 123, 123–125, 125, 125–126, 129, 132, 139–140, 149
src/views
   Dashboard.vue0%0%0%0%101, 106, 106, 106–107, 109, 115, 119–120, 128, 131, 135–137, 139, 142–143, 146–147, 147–148, 152–153, 153–154, 156, 158–159, 159, 159, 159, 159–161, 161, 161, 161, 161–163, 166, 171, 171, 171, 173–174, 174, 174–175, 177, 177–179, 183, 187, 190, 190, 190, 193, 193, 193, 198, 201, 201, 201, 208, 211, 211, 211, 216, 216, 216, 22, 221, 221, 221, 226, 229, 23, 232, 235, 235, 235, 238, 238, 238, 24, 241, 244, 247, 25–30, 47, 54, 54, 61, 64–65, 77–78, 78, 78, 81, 84, 84, 84–86, 86, 86–88, 88–89, 91, 94, 94, 94–96, 96, 96–98, 98–99
   ProjectsTab.vue90.91%100%83.33%90.48%74–75

@github-actions
Copy link

PHP Code Coverage

Coverage after merging loading-icon into master will be
43.56%
Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
lib/AppInfo
   Application.php0%100%0%0%101, 105, 49, 51–52, 54–56, 59–60, 63–65, 67, 70–71, 74–77, 79–84, 86, 88, 91, 95, 99
lib/BackgroundJob
   CheckNotifications.php0%100%0%0%48, 50, 52–53, 61–62
lib/Controller
   ConfigController.php60.71%100%60%60.76%105–106, 108, 184–189, 191–194, 196, 198, 71–72, 74, 76–78, 80–85, 87, 91–92, 94
   OpenProjectAPIController.php96.43%100%85.71%97.96%77
lib/Dashboard
   OpenProjectWidget.php0%100%0%0%106–109, 61–64, 71, 78, 85, 92, 99
lib/Listener
   LoadSidebarScript.php0%100%0%0%59–61, 65–66, 68, 70, 72–74, 76–78
lib/Notification
   Notifier.php0%100%0%0%100, 110, 112, 114, 47–50, 60, 69, 80, 82, 85, 87–92
lib/Search
   OpenProjectSearchProvider.php0%100%0%0%102, 109–110, 113–116, 118–119, 123, 127–131, 133–135, 138–139, 141–142, 146–147, 157, 169, 177, 180, 183, 192, 195, 206, 71–75, 82, 89, 97, 99
   OpenProjectSearchResultEntry.php100%100%100%100%
lib/Service
   OpenProjectAPIService.php58.80%100%66.67%58.21%105–114, 116–120, 122–127, 129–130, 133–136, 143, 150–154, 164–165, 167–171, 173–174, 182–186, 365–366, 368, 374, 380–385, 387, 393, 421–424, 427–428, 430, 432–433, 455–456, 463, 466–469, 471, 477, 481–483, 95–98
lib/Settings
   Admin.php0%100%0%0%25–26, 33–35, 37, 42–43, 47, 51
   AdminSection.php0%100%0%0%19–20, 29, 39, 48, 55
   Personal.php0%100%0%0%38–41, 48–52, 55, 57, 65–66, 70, 74
   PersonalSection.php0%100%0%0%19–20, 29, 39, 48, 55

@individual-it individual-it merged commit c23e2d6 into master Feb 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the loading-icon branch February 24, 2022 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants