Skip to content

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 7, 2020

  • unvendor vue and vue-calendar-heatmap
  • remove unused moment.js leftover from previous heatmap version
  • ensure webpack loads the full version of vue
  • fix vue devmode warning related to 'searchLimit' type
  • update vue from 2.6.6 to 2.6.11

I wanted to name the chunk heatmap.js but adblockers don't like that filename [1].

[1] https://github.com/easylist/easylist/blob/3899d5dff33216c0bc64f09ff15d376f346d3e33/easyprivacy/easyprivacy_general.txt#L2095

@silverwind
Copy link
Member Author

There are more Vue warnings visible when webpack builds in development mode. I want to eventually enable us to use that mode, but right now I can not get bindata-less builds to work at all for some reason which is a requirement for that.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 7, 2020
@silverwind silverwind force-pushed the vueheat branch 3 times, most recently from e207386 to 9b3a311 Compare February 7, 2020 22:34
@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #10188 into master will decrease coverage by 0.01%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10188      +/-   ##
==========================================
- Coverage   43.69%   43.68%   -0.02%     
==========================================
  Files         586      586              
  Lines       81354    81386      +32     
==========================================
+ Hits        35547    35552       +5     
- Misses      41410    41431      +21     
- Partials     4397     4403       +6
Impacted Files Coverage Δ
routers/user/auth_openid.go 0% <0%> (ø) ⬆️
routers/user/setting/profile.go 39.47% <0%> (-0.8%) ⬇️
models/login_source.go 25.45% <0%> (-0.18%) ⬇️
routers/api/v1/repo/migrate.go 53.93% <0%> (-0.67%) ⬇️
routers/api/v1/admin/org.go 46.73% <0%> (-0.52%) ⬇️
routers/user/auth.go 11.4% <0%> (-0.04%) ⬇️
routers/api/v1/org/org.go 74.45% <0%> (-0.33%) ⬇️
routers/admin/users.go 28.94% <0%> (-0.39%) ⬇️
modules/auth/pam/pam_stub.go 0% <0%> (ø) ⬆️
routers/api/v1/admin/user.go 30.09% <0%> (-0.1%) ⬇️
... and 19 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 b50e1d4...8133ea8. Read the comment docs.

@silverwind
Copy link
Member Author

The heatmap should probably be moved to a .vue file at some point and loaded via vue-loader but I don't have the motivation to do that as I'm not that much of a Vue fan 😉.

@lunny lunny added type/refactoring Existing code has been cleaned up. There should be no new functionality. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile labels Feb 8, 2020
@silverwind
Copy link
Member Author

@lafriks any feedback? If you like, I can try porting heatmap to a .vue file but would prefer if you do it because vue is black magic to me. I think we can probably streamline the vue initialization parts to a shared vue rendering function.

@silverwind silverwind changed the title move vue and vue-calendar-heatmap to webpack move vue and vue-calendar-heatmap to npm/webpack Feb 11, 2020
@lafriks
Copy link
Member

lafriks commented Feb 11, 2020

Looks good, it can be converted to Vue in other pr

@silverwind silverwind force-pushed the vueheat branch 2 times, most recently from c085ef9 to dd8016b Compare February 13, 2020 23:10
@silverwind
Copy link
Member Author

Rebased. This is good to go from my perspective.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 14, 2020
@gary-kim
Copy link
Member

The heatmap should probably be moved to a .vue file at some point and loaded via vue-loader but I don't have the motivation to do that as I'm not that much of a Vue fan wink.

Okay, I can do that part. Added to todo.

@silverwind
Copy link
Member Author

@gary-kim I'd prefer if we create a single vue rendering function to be used by all the vue components. I think the current code initializes vue in 2 or 3 different places, which seems like useless code duplication to me. Maybe create src/vue.js and export that rendering function there.

@gary-kim
Copy link
Member

I like that idea. Make a single rendering function that imports all the Vue components and calls $mount on them.

@lunny lunny added this to the 1.12.0 milestone Feb 18, 2020
@silverwind
Copy link
Member Author

Rebased

@silverwind
Copy link
Member Author

Any more reviews here?

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 23, 2020
- unvendor vue and vue-calendar-heatmap
- remove unused moment.js leftover from previous heatmap version
- ensure webpack loads the full version of vue
- fix vue devmode warning related to 'searchLimit' type

I wanted to name the chunk heatmap.js but adblockers don't like that
filename [1].

[1] https://github.com/easylist/easylist/blob/3899d5dff33216c0bc64f09ff15d376f346d3e33/easyprivacy/easyprivacy_general.txt#L2095
@silverwind
Copy link
Member Author

Rebased once more, good to go.

@zeripath zeripath merged commit 062f351 into go-gitea:master Feb 23, 2020
@silverwind silverwind deleted the vueheat branch February 23, 2020 21:37
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants