pleroma/pleroma-fe
Watch
2
2
Fork
You've already forked pleroma-fe
 10
Code Issues 351 Pull requests 26 Projects Releases 5 Packages 1 Wiki Activity

Feature/add sticker picker #2152

Closed
absturztaube wants to merge 25 commits from gitlab-mr-iid-885 into develop
pull from: gitlab-mr-iid-885
merge into: pleroma:develop
Conversation 44 Commits 25 Files changed 8 +210 -4
absturztaube commented 2019-07-17 18:08:44 +00:00
Member
Copy link

adds sticker picker to post status form as well as 2 stickerpacks

adds sticker picker to post status form as well as 2 stickerpacks
kaniini commented 2019-07-17 20:03:18 +00:00
Member
Copy link

use bare fetch. we have fetch in scope.

use bare fetch. we have fetch in scope.
kaniini commented 2019-07-17 20:08:44 +00:00
Member
Copy link

regarding the sticker packs, anything we distribute with Pleroma, we need to know what the copyright + redistribution terms are. can you get that information for us?

regarding the sticker packs, anything we distribute with Pleroma, we need to know what the copyright + redistribution terms are. can you get that information for us?
hj commented 2019-07-17 20:40:16 +00:00
Member
Copy link

Object.entries

Object.entries
hj commented 2019-07-17 20:41:53 +00:00
Member
Copy link
              :class="{ selected: stickerPickerVisible }"
```suggestion :class="{ selected: stickerPickerVisible }" ```
hj commented 2019-07-17 20:42:24 +00:00
Member
Copy link

or just use v-if?

or just use v-if?
hj commented 2019-07-17 20:44:34 +00:00
Member
Copy link

not sure, eslint might disallow this, we probably should allow it tho

not sure, eslint might disallow this, we probably should allow it tho
hj commented 2019-07-17 20:47:25 +00:00
Member
Copy link

i feel like this is a huge wraparound, if you already know the url you probably can already use it, no need to reupload it, just reference the url

i feel like this is a huge wraparound, if you already know the url you probably can already use it, no need to reupload it, just reference the url
hj commented 2019-07-17 20:48:16 +00:00
Member
Copy link

we already have a tab-switcher component for such things

we already have a tab-switcher component for such things
hj commented 2019-07-17 20:53:39 +00:00
Member
Copy link

icon is visible even if there are no stickers

icon is visible even if there are no stickers
hj commented 2019-07-17 21:00:09 +00:00
Member
Copy link

image

some UX/UI concerns:

  • de-saturating icons makes it harder to see and recognize them, glow would probably be better
  • no meaningful names or hover text for anything, filenames are garbage too
  • having stickers.json and other stuff in git makes it harder to customize it for admins.
  • a bit weird and not clear convention - list references tabs and it's assumes that there should be tab.png and pack.json, directly referencing pack.json and listing tab icon in pack.json would be better.
  • grid is somewhat ugly with that empty space on the right
![image](/attachments/969109de-dcbb-4bed-a5f8-2ae6212cef2e) some UX/UI concerns: * de-saturating icons makes it harder to see and recognize them, glow would probably be better * no meaningful names or hover text for anything, filenames are garbage too * having stickers.json and other stuff in git makes it harder to customize it for admins. * a bit weird and not clear convention - list references tabs and it's assumes that there should be tab.png and pack.json, directly referencing pack.json and listing tab icon in pack.json would be better. * grid is somewhat ugly with that empty space on the right
image.png
185 KiB
hj commented 2019-07-17 21:02:17 +00:00
Member
Copy link

also don't like including shitton of images in the repo without LFS, on top of that, as i mentioned before having stuff in git makes it harder to customize it for admins. and lastly - probably should be either in separate repo like our emojis pseudoAPT repo or probably even backend.

also don't like including shitton of images in the repo without LFS, on top of that, as i mentioned before having stuff in git makes it harder to customize it for admins. and lastly - probably should be either in separate repo like our emojis pseudoAPT repo or probably even backend.
hj commented 2019-07-17 21:03:12 +00:00
Member
Copy link

space after comma please

space after comma please
hj commented 2019-07-17 21:04:18 +00:00
Member
Copy link

also why not fetch everything at once like we do with emojis? this makes fetch every time you change tab.

also why not fetch everything at once like we do with emojis? this makes fetch every time you change tab.
hj commented 2019-07-17 21:05:05 +00:00
Member
Copy link

i don't think you actually need it, you're using arrow functions

i don't think you actually need it, you're using arrow functions
hj commented 2019-07-17 21:07:17 +00:00
Member
Copy link

this doesn't work very well with transparent-ish themes and also overall a bad choice

this doesn't work very well with transparent-ish themes and also overall a bad choice
hj commented 2019-07-17 21:10:14 +00:00
Member
Copy link

miko fox redistribution terms are 1 euro on line sticker store, lol https://store.line.me/stickershop/product/1259582

miko fox redistribution terms are 1 euro on line sticker store, lol https://store.line.me/stickershop/product/1259582
hj commented 2019-07-17 21:11:41 +00:00
Member
Copy link

the other one is 2 euros wwwwwwwwwwwwww https://store.line.me/stickershop/product/1333431/en

the other one is 2 euros wwwwwwwwwwwwww https://store.line.me/stickershop/product/1333431/en
kaniini commented 2019-07-17 23:14:21 +00:00
Member
Copy link

sounds like we probably shouldn't redistribute them then :)

maybe a pleroma-tan sticker pack would be best to include :)

sounds like we probably shouldn't redistribute them then :) maybe a pleroma-tan sticker pack would be best to include :)
absturztaube commented 2019-07-18 13:07:01 +00:00
Author
Member
Copy link

this is a huge wraparound, yes. i did it this way because statusPosterService.postStatus just uses the ids of attachment and i couldn't think of another way to attach the sticker to the post.
am i missing something here?

this is a huge wraparound, yes. i did it this way because statusPosterService.postStatus just uses the ids of attachment and i couldn't think of another way to attach the sticker to the post. am i missing something here?
absturztaube commented 2019-07-18 13:11:52 +00:00
Author
Member
Copy link

tab-switcher doesn't provide a possibility to use images as tab labels yet. should i add it?

edit: sorry, replied to the wrong comment

tab-switcher doesn't provide a possibility to use images as tab labels yet. should i add it? edit: sorry, replied to the wrong comment
hj commented 2019-07-18 13:31:40 +00:00
Member
Copy link

not entirely sure, but i'd mess around to see what could be done. otherwise leave a TODO there

not entirely sure, but i'd mess around to see what could be done. otherwise leave a TODO there
hj commented 2019-07-18 13:32:21 +00:00
Member
Copy link

yeah, that would be a nice option.

yeah, that would be a nice option.
absturztaube commented 2019-07-21 19:04:39 +00:00
Author
Member
Copy link

i removed the sticker packs

i removed the sticker packs
absturztaube commented 2019-07-21 19:06:12 +00:00
Author
Member
Copy link

filenames are garbage but we can't really control them because they are derived from the sticker packs.

filenames are garbage but we can't really control them because they are derived from the sticker packs.
absturztaube commented 2019-07-21 19:06:52 +00:00
Author
Member
Copy link

@hj review please

@hj review please
absturztaube commented 2019-07-21 19:46:47 +00:00
Author
Member
Copy link

i noticed that the tab-switcher loads the content of all its tabs at once. because i have many stickerpacks on my instance it produces tons of requests for fetching the images (which takes quite some time). does the tab-switcher support loading content only if tab is opened?

i noticed that the tab-switcher loads the content of all its tabs at once. because i have many stickerpacks on my instance it produces tons of requests for fetching the images (which takes quite some time). does the tab-switcher support loading content only if tab is opened?
hj commented 2019-07-21 20:05:33 +00:00
Member
Copy link

it does, but i'm not sure if i added tab-switch hooks as i wanted or not.

it does, but i'm not sure if i added tab-switch hooks as i wanted or not.
hj commented 2019-07-21 20:06:02 +00:00
Member
Copy link

we can enforce good practice of naming them correctly at least.

we can enforce good practice of naming them correctly at least.
hj commented 2019-07-21 20:09:41 +00:00
Member
Copy link

instead of having an array and pushing into it you can just do

const stickers = Object.entries(values).map(([key, value]) => {
  return {
    whatever: data.you.want
  }
})
instead of having an array and pushing into it you can just do ```js const stickers = Object.entries(values).map(([key, value]) => { return { whatever: data.you.want } }) ```
hj commented 2019-07-21 20:12:16 +00:00
Member
Copy link
          pack: name,
          path,
          meta
```suggestion:-2 pack: name, path, meta ```
hj commented 2019-07-21 20:17:21 +00:00
Member
Copy link

maybe add console.warn at least?

maybe add console.warn at least?
hj commented 2019-07-21 20:27:02 +00:00
Member
Copy link

image

stickers/poll icons look weird on mobile

![image](/attachments/e4a9875b-dc65-470d-835e-c431bf43092e) stickers/poll icons look weird on mobile
image.png
91 KiB
hj commented 2019-07-21 20:29:47 +00:00
Member
Copy link

i'd use just v-if here instead of inside the component

i'd use just v-if here instead of inside the component
hj commented 2019-07-21 20:34:37 +00:00
Member
Copy link

overall looks good, except for broken tab icons, but i just reused stickers from earlier commit, not sure if you changed format.

and yeah, format, we probably still need to discuss format and distribution and how backend gonna serve those, because backend doesn't like including stuff in frontend.

overall looks good, except for broken tab icons, but i just reused stickers from earlier commit, not sure if you changed format. and yeah, format, we probably still need to discuss format and distribution and how backend gonna serve those, because backend doesn't like including stuff in frontend.
succfemboi commented 2019-07-22 09:15:40 +00:00
Member
Copy link

couldn't lazyloading everything fix this?

couldn't lazyloading everything fix this?
hj commented 2019-07-22 09:30:58 +00:00
Member
Copy link

not sure what you mean by that

not sure what you mean by that
absturztaube commented 2019-07-22 13:38:29 +00:00
Author
Member
Copy link

@hj review please

some notes:

  • the render-only-focused attribute on the tab-switcher did the job to prevent loading all sticker images at once
  • had to move the sorting to after_store because it caused issues if 2 pickers are opened at once (pack() in sticker_picker.js got called endlessly in that case)
  • as for the broken tab icons, i changed the format a bit so pack.json references the tab icon
{
  "author": "absturztaube",
  "title": "Miko-san of foxes",
  "tabIcon": "tab.png",
  "stickers": [
@hj review please some notes: - the render-only-focused attribute on the tab-switcher did the job to prevent loading all sticker images at once - had to move the sorting to after_store because it caused issues if 2 pickers are opened at once (pack() in sticker_picker.js got called endlessly in that case) - as for the broken tab icons, i changed the format a bit so pack.json references the tab icon ``` { "author": "absturztaube", "title": "Miko-san of foxes", "tabIcon": "tab.png", "stickers": [ ```
kaniini commented 2019-07-24 00:05:28 +00:00
Member
Copy link

I've deployed this on pleroma.site and it seems to be working well. What blocks merging this?

I've deployed this on pleroma.site and it seems to be working well. What blocks merging this?
hj commented 2019-07-24 06:36:08 +00:00
Member
Copy link

gonna check it one more time today

gonna check it one more time today
hj commented 2019-07-24 18:12:52 +00:00
Member
Copy link

resolved

resolved
hj commented 2019-07-24 18:15:48 +00:00
Member
Copy link

remove the eslint-disable comment it should not be triggering now

remove the eslint-disable comment it should not be triggering now
hj commented 2019-07-24 18:21:03 +00:00
Member
Copy link

looks good, just need to remove that one eslint-disable line (dunno if it would still yell at you or not tho)

even if format isn't perfect we can always make a v2 of it in the future

looks good, just need to remove that one eslint-disable line (dunno if it would still yell at you or not tho) even if format isn't perfect we can always make a v2 of it in the future
absturztaube commented 2019-07-24 18:47:16 +00:00
Author
Member
Copy link

it didn't yell at me.

it didn't yell at me.
hj commented 2019-07-24 19:34:05 +00:00
Member
Copy link

moce

moce

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
BE Request

   
Backlog

   
Better behavior

   
Blocked by deps

   
Bug

   
Code Review

   
Discussion

   
Doing

   
Feature Request / Enhancement

   
In the shining bright future maybe

   
It's complicated

   
MS Edge

   
Mememoon (or other niche browsers)

   
Missing API

   
Missing feature

   
Missing l10n/i18n

   
NL1
Instance Admin Dashboard

  
NL2
Improved Mobile Experience

  
NL4
Improved instance customization and QoL options

  
Need to verify on develop

   
RELEASE BLOCKER

   
Reassign or Close

   
Refactor

   
Regression

   
Safari

   
To Do

   
User story

   
Waiting on godot

   
accessibility
https://git.pleroma.social/pleroma/pleroma-fe/-/snippets/7746

  
better documentation

   
chore

   
confirmed

   
easy ticket

   
incident
Denotes a disruption to IT services and the associated issues require immediate attention

  
l10n update

   
mastoapi

   
mobile

   
needs design

   
needs-info

   
needs-review

   
stupid
No labels
BE Request
Backlog
Better behavior
Blocked by deps
Bug
Code Review
Discussion
Doing
Feature Request / Enhancement
In the shining bright future maybe
It's complicated
MS Edge
Mememoon (or other niche browsers)
Missing API
Missing feature
Missing l10n/i18n
NL1
NL2
NL4
Need to verify on develop
RELEASE BLOCKER
Reassign or Close
Refactor
Regression
Safari
To Do
User story
Waiting on godot
accessibility
better documentation
chore
confirmed
easy ticket
incident
l10n update
mastoapi
mobile
needs design
needs-info
needs-review
stupid
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
pleroma/pleroma-fe!2152
No description provided.