Skip to content

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 14, 2021

ref nextcloud/app-tutorial#319

  • Feel free to revert all my github actions changes
  • There is ONE linting issue that require a bit more attention
  • Builds fine BUT didn't look extensively at the ui (seems good at first sight)

@skjnldsv skjnldsv requested a review from ChristophWurst June 14, 2021 10:14
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #3215 (578e97d) into master (1b9535d) will decrease coverage by 0.59%.
The diff coverage is 0.00%.

❗ Current head 578e97d differs from pull request most recent head 3c98f7a. Consider uploading reports for the commit 3c98f7a to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3215      +/-   ##
============================================
- Coverage     28.89%   28.30%   -0.60%     
  Complexity      122      122              
============================================
  Files           158      157       -1     
  Lines          5741     5854     +113     
  Branches        844      811      -33     
============================================
- Hits           1659     1657       -2     
- Misses         4082     4197     +115     
Flag Coverage Δ
javascript 22.87% <0.00%> (-0.53%) ⬇️
php 94.58% <ø> (ø)

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

Impacted Files Coverage Δ
...NavigationHeader/AppNavigationHeaderDatePicker.vue 0.00% <0.00%> (ø)
src/components/AppNavigation/CalendarList.vue 0.00% <ø> (ø)
...ts/AppNavigation/CalendarList/CalendarListItem.vue 0.00% <0.00%> (ø)
...nts/AppNavigation/CalendarList/CalendarListNew.vue 0.00% <0.00%> (ø)
...Navigation/CalendarList/PublicCalendarListItem.vue 0.00% <0.00%> (ø)
...components/AppNavigation/CalendarList/Trashbin.vue 0.00% <ø> (ø)
src/components/AppNavigation/Settings.vue 0.00% <0.00%> (ø)
...ponents/AppNavigation/Settings/ImportScreenRow.vue 0.00% <ø> (ø)
src/components/CalendarGrid.vue 0.00% <ø> (ø)
...rc/components/Editor/Alarm/AlarmTimeUnitSelect.vue 0.00% <ø> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b9535d...3c98f7a. Read the comment docs.

@skjnldsv skjnldsv added 2. developing Work in progress 3. to review Waiting for reviews labels Jun 14, 2021
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

I just tested a bit and the UI seems to work fine.

I have no idea what the linter is trying to tell us:

/var/www/nextcloud/dev_apps/calendar/src/components/AppNavigation/Settings/ImportScreenRow.vue
  71:20  error  Unexpected side effect in "calendars" computed property  vue/no-side-effects-in-computed-properties

I don't see a side effect in the mentioned computed property 🤷‍♂️

@skjnldsv
Copy link
Member Author

I don't see a side effect in the mentioned computed property

Arrays are passed as references, pushing an item into it will update the orignal store value

@st3iny
Copy link
Member

st3iny commented Jun 15, 2021

I don't see a side effect in the mentioned computed property

Arrays are passed as references, pushing an item into it will update the orignal store value

Sure but the linter complains even if I remove the push or slice the array right after obtaining it from the getter.

@skjnldsv
Copy link
Member Author

Sure but the linter complains even if I remove the push or slice the array right after obtaining it from the getter.

image
Indeed

@st3iny st3iny force-pushed the feat/deps-npm7 branch 2 times, most recently from 578e97d to 56b147e Compare June 25, 2021 09:10
@st3iny st3iny self-assigned this Jun 25, 2021
@st3iny st3iny added dependencies Pull requests that update a dependency file technical debt and removed 2. developing Work in progress labels Jun 25, 2021
@st3iny
Copy link
Member

st3iny commented Jun 25, 2021

I resolved all conflicts, rebased and fixed all pending issues. Let's get this merged quickly before more conflicts arise and I have to start over again.

@ChristophWurst The required checks have to be adjusted in the repository settings. I renamed the build jobs for consistency with the js tests jobs.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

👍 but didn' test

@st3iny st3iny added this to the v2.4.0 milestone Jun 25, 2021
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

I did some extensive testing and didn't encounter a broken feature.

skjnldsv added 3 commits June 25, 2021 14:38
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: Richard Steinmetz <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: Richard Steinmetz <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: Richard Steinmetz <[email protected]>
@ChristophWurst ChristophWurst merged commit c03a529 into master Jun 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/deps-npm7 branch June 25, 2021 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews dependencies Pull requests that update a dependency file technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants