Skip to content

Conversation

@lafriks
Copy link
Member

@lafriks lafriks commented Nov 26, 2018

Looks about the same but fixes problems reported in #5191. Also removed setting for range color and instead moved it to less/css so that colors can be specified in theme css

@lafriks lafriks added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels Nov 26, 2018
@lafriks lafriks added this to the 1.7.0 milestone Nov 26, 2018
@pjebs
Copy link

pjebs commented Nov 26, 2018

Are we favouring Vue over react for future?

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 26, 2018
@techknowlogick
Copy link
Member

@pjebs yes

@codecov-io
Copy link

codecov-io commented Nov 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@c03a9b3). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5401   +/-   ##
=========================================
  Coverage          ?   37.44%           
=========================================
  Files             ?      312           
  Lines             ?    46507           
  Branches          ?        0           
=========================================
  Hits              ?    17416           
  Misses            ?    26599           
  Partials          ?     2492
Impacted Files Coverage Δ
modules/setting/setting.go 48.06% <ø> (ø)
modules/templates/helper.go 48.95% <ø> (ø)

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 c03a9b3...e5f77f7. Read the comment docs.

@bkcsoft bkcsoft 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 Nov 27, 2018
@bkcsoft bkcsoft 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 Nov 27, 2018
@jonasfranz jonasfranz merged commit e09fe48 into go-gitea:master Nov 27, 2018
@ghost
Copy link

ghost commented Nov 27, 2018

@lafriks I don't know why this happened.
1

console.log(this.colorRange); return:

0: ""
1: ""
2: ""
3: ""
4: ""
5: ""

Chrome 70.0.3538.110

@lunny
Copy link
Member

lunny commented Nov 27, 2018

@lafriks I can confirm @yasuokav 's report.

@sapk
Copy link
Member

sapk commented Nov 27, 2018

@lunny @yasuokav Can you send the result of http://jsfiddle.net/3wz4xdgq/13/ in your browser ? I suspect getComputedStyle to not work the same depending of browser.

@sapk
Copy link
Member

sapk commented Nov 27, 2018

For me on Firefox :

test backgroundColor: "rgb(255, 0, 0)"
clone backgroundColor: "rgb(255, 0, 0)"
new backgroundColor: "rgb(255, 0, 0)"
non-existing backgroundColor: "rgb(255, 0, 0)"

@sapk
Copy link
Member

sapk commented Nov 27, 2018

On chrome :

test backgroundColor: "rgb(255, 0, 0)"
clone backgroundColor: ""
new backgroundColor: ""
non-existing backgroundColor: ""

That why it doesn't work ... Please use a browser that support standard 😆
Just kidding, we just have to find a workaround getComputedStyle() variant.

@sapk
Copy link
Member

sapk commented Nov 27, 2018

Found the bug on chrome and it is still open : https://bugs.chromium.org/p/chromium/issues/detail?id=558165

@ghost
Copy link

ghost commented Nov 27, 2018

@sapk The results are the same as yours

@tonivj5
Copy link
Contributor

tonivj5 commented Nov 27, 2018

Maybe could we use a polyfill for getComputedStyle?

@tonivj5 tonivj5 mentioned this pull request Nov 27, 2018
7 tasks
@lafriks lafriks deleted the feat/vue-heatmap branch November 27, 2018 22:52
@lafriks
Copy link
Member Author

lafriks commented Nov 27, 2018

ah shity browser standard support :D

@sapk
Copy link
Member

sapk commented Nov 28, 2018

@xxxTonixxx I don't think we can have a polyfill since the func is already defined. The workaround that I see is creating a hidden el with the class un order for webkit to compute the style. An other would be to define the variable in the theme or keep like before a config that define the JS var.

@lunny lunny mentioned this pull request Nov 29, 2018
7 tasks
@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. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/bug 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.

9 participants