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

PR@2473: Add new background pattern category "wallpaper" #2689

Merged
merged 6 commits into from
Nov 7, 2018
Merged

PR@2473: Add new background pattern category "wallpaper" #2689

merged 6 commits into from
Nov 7, 2018

Conversation

kuhnchris
Copy link
Contributor

Description

Adds new background pattern category

Checklist
  • changes don't break existing behavior
Affected core subsystem(s)

dashboard

Testing

done

Refers/Fixes

Fixes #2473

@@ -1,5 +1,6 @@
let openSection;
const layers = [
'Wallpaper',

Choose a reason for hiding this comment

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

Expected indentation of 2 spaces but found 1 tab. (indent)

@@ -204,6 +205,7 @@ function setOption(option, value, target) {
case 'Nose':
case 'Ears':
case 'Mouth':
case 'Wallpaper':

Choose a reason for hiding this comment

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

Expected indentation of 4 spaces but found 1 tab. (indent)

}, {
'name': 'Wallpaper',
'title': 'Pick some swag for your back',
'options': ('anchors','jigsaw')

Choose a reason for hiding this comment

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

E231 missing whitespace after ','

'title': 'Pick some swag for your back',
'options': ('anchors','jigsaw')
}
],

Choose a reason for hiding this comment

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

E122 continuation line missing indentation or outdented

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

@kuhnchris could you address the lint comments + run svgo to compress the svg + show a demo of the added wallpaper :)

@owocki
Copy link
Contributor

owocki commented Nov 6, 2018

make fix will handle many of the linting issues!

also need to resolve the merge conflicts

@kuhnchris
Copy link
Contributor Author

Yeah I'll take care of that ASAP, thanks for pointing it out!

@kuhnchris
Copy link
Contributor Author

image
image
image

@owocki
Copy link
Contributor

owocki commented Nov 6, 2018

looking good!

@codecov
Copy link

codecov bot commented Nov 6, 2018

Codecov Report

Merging #2689 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2689      +/-   ##
==========================================
- Coverage   29.89%   29.88%   -0.01%     
==========================================
  Files         162      162              
  Lines       13050    13052       +2     
  Branches     1742     1743       +1     
==========================================
  Hits         3901     3901              
- Misses       9025     9027       +2     
  Partials      124      124
Impacted Files Coverage Δ
app/avatar/utils.py 18.43% <0%> (ø) ⬆️
app/kudos/models.py 49.67% <0%> (-0.65%) ⬇️

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 701183b...ad50c57. Read the comment docs.

@thelostone-mc thelostone-mc merged commit cb11d42 into gitcoinco:master Nov 7, 2018
@kuhnchris
Copy link
Contributor Author

Thanks @thelostone-mc for fixing that!

@owocki
Copy link
Contributor

owocki commented Nov 9, 2018

@kuhnchris i wonder what i'm doing wrong with the new assets i added ...

#2746

@kuhnchris
Copy link
Contributor Author

Wallpapers are a very... special thing. SVG has some kind of viewport defined at times, we need to check what you background did and check that one

@owocki
Copy link
Contributor

owocki commented Nov 9, 2018

@kuhnchris here is a SVG i commited : https://github.com/gitcoinco/web/blob/master/app/assets/v2/images/avatar/Wallpaper/gears.svg

there are others in the same directory as it

what are the specs of the viewport i should set?

@kuhnchris
Copy link
Contributor Author

Hi there, I never figured that out aswell, but it seems to vary wildly between template and display in avatar builder, hence my request if we could make a unified system that uses the same components instead of squashing them.
For repeating background we actually need to define a pattern as seen in the anchors.svg, it's complicated and has to be done by hand. Sadly I know that sucks.

What focus do you want @owocki : rewriting the avatar generation or fixing those SVGs? I think that the problems only continue to grow with the current system...

@owocki
Copy link
Contributor

owocki commented Nov 12, 2018

per convo on slack, @kuhnchris is gonna submit a short term fix now and we'll break the larger scope into another ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants