pleroma/pleroma-fe
2
1
Fork 2
Code Issues 345 Pull requests 30 Projects Releases Packages Wiki Activity

Allow remove of banner, avatar images issue #353 #2146

Closed
wyatt777 wants to merge 22 commits from gitlab-mr-iid-879 into develop
pull from: gitlab-mr-iid-879
merge into: pleroma:develop
Conversation 67 Commits 22 Files changed 8 +171 -22
wyatt777 commented 2019-07-10 07:11:32 +00:00
Member
Copy link

Added in new backend API calls to do this.

Added in new backend API calls to do this.
shpuld commented 2019-07-10 16:40:16 +00:00
Owner
Copy link

dangerous change, why?

dangerous change, why?
shpuld commented 2019-07-10 16:40:52 +00:00
Owner
Copy link

hard coded file names seems like quite the no-no

hard coded file names seems like quite the no-no
shpuld commented 2019-07-10 16:42:10 +00:00
Owner
Copy link

this isn't a clone

this isn't a clone
shpuld commented 2019-07-10 16:45:23 +00:00
Owner
Copy link

seems like a very hacky way of achieving the reset tbh

seems like a very hacky way of achieving the reset tbh
hj commented 2019-07-10 20:36:46 +00:00
Owner
Copy link

also indexOf === -1 instead of includes()

also `indexOf === -1` instead of `includes()`
hj commented 2019-07-10 20:37:19 +00:00
Owner
Copy link

also if (boolean) { return true } else { return false } all over the place

also `if (boolean) { return true } else { return false }` all over the place
hj commented 2019-07-10 20:39:26 +00:00
Owner
Copy link

i'd rather make user avatar component handle undefined correctly instead of (temporarily?) setting it to semi-hardcoded string

i'd rather make user avatar component handle undefined correctly instead of (temporarily?) setting it to semi-hardcoded string
hj commented 2019-07-10 20:39:58 +00:00
Owner
Copy link

what's a "blank user"? is it a BE bug?

what's a "blank user"? is it a BE bug?
hj commented 2019-07-10 20:41:37 +00:00
Owner
Copy link

"the X image" sounds weird to me

"the X image" sounds weird to me
hj commented 2019-07-10 20:43:48 +00:00
Owner
Copy link

hold on, those aren't in Mastodon API, are those pleroma extensions? why are they in /api/v1/ ?

hold on, those aren't in Mastodon API, are those pleroma extensions? why are they in /api/v1/ ?
feld commented 2019-07-10 22:17:37 +00:00
Owner
Copy link

Yes these are Pleroma extensions to the Mastodon API

Yes these are Pleroma extensions to the Mastodon API
wyatt777 commented 2019-07-10 22:22:19 +00:00
Author
Member
Copy link

This is min-width for buttons only on settings. Did you think dangerous because you thought it was for all buttons? That would be. The smallest button is Submit and it looks fine at this width.

The new button Reset Avatar looks better at this width in English. Would need to check it for all other languages.

This is min-width for buttons only on settings. Did you think dangerous because you thought it was for all buttons? That would be. The smallest button is Submit and it looks fine at this width. The new button Reset Avatar looks better at this width in English. Would need to check it for all other languages.
wyatt777 commented 2019-07-10 22:25:02 +00:00
Author
Member
Copy link

It sends back User data with everything as undefined. Will double check if it is something I did and if not post it as an issue on the merged BE branch. I would really like it to return the same way that the other API works.

It sends back User data with everything as undefined. Will double check if it is something I did and if not post it as an issue on the merged BE branch. I would really like it to return the same way that the other API works.
wyatt777 commented 2019-07-10 22:31:05 +00:00
Author
Member
Copy link

Fixed the two issue by @hj . @shpuld What do you recommend?

Fixed the two issue by @hj . @shpuld What do you recommend?
wyatt777 commented 2019-07-10 23:10:19 +00:00
Author
Member
Copy link

I looked into user_avatar component. It seems to not be working..

I made a fix locally and realized changing that will only affect one location. Changing profile_image_url will change Avatars in all locations. The original request should be another issue for that component.

I looked into user_avatar component. It seems to not be working.. I made a fix locally and realized changing that will only affect one location. Changing `profile_image_url` will change Avatars in all locations. The original request should be another issue for that component.
wyatt777 commented 2019-07-10 23:13:51 +00:00
Author
Member
Copy link

Ha! Any suggestions? Just avatar?

Ha! Any suggestions? Just avatar?
wyatt777 commented 2019-07-10 23:18:42 +00:00
Author
Member
Copy link

Actually images/avi.png is hardcoded in multiple places across FE. I guess we could eventually move it to a setting and change all of those locations.

Actually `images/avi.png` is hardcoded in multiple places across FE. I guess we could eventually move it to a setting and change all of those locations.
wyatt777 commented 2019-07-10 23:28:04 +00:00
Author
Member
Copy link

Please look at submitAvatar. I would like to set it like that. But the backend is returning a user with everything undefined. For now, I will make it just change the user state.

Please look at submitAvatar. I would like to set it like that. But the backend is returning a user with everything undefined. For now, I will make it just change the user state.
wyatt777 commented 2019-07-10 23:30:54 +00:00
Author
Member
Copy link

Moving this to BE. Seems to be a bug there.

Moving this to BE. Seems to be a bug there.
wyatt777 commented 2019-07-11 00:05:04 +00:00
Author
Member
Copy link

@shpuld changed the hardcoded places for /images/avi.png across FE. @hj Switched to includes() and made boolean switches one liners.

@shpuld changed the hardcoded places for `/images/avi.png` across FE. @hj Switched to `includes()` and made boolean switches one liners.
hj commented 2019-07-11 02:53:15 +00:00
Owner
Copy link

yea

yea
wyatt777 commented 2019-07-11 03:17:54 +00:00
Author
Member
Copy link

k

k
wyatt777 commented 2019-07-11 05:53:19 +00:00
Author
Member
Copy link

pushed.

pushed.
hj commented 2019-07-11 09:49:58 +00:00
Owner
Copy link

"looks better" is very questionable, and should be accompanied with screenshots and probably other relevant changes. Which seems like out of scope for this task

"looks better" is very questionable, and should be accompanied with screenshots and probably other relevant changes. Which seems like out of scope for this task
hj commented 2019-07-11 09:54:27 +00:00
Owner
Copy link

what do you mean, one location? you should change profile_image_url to undefined as well as other things like banner and background. Relevant components must be taught how to handle this situation.

what do you mean, one location? you should change `profile_image_url` to `undefined` as well as other things like banner and background. Relevant components must be taught how to handle this situation.
hj commented 2019-07-11 09:56:17 +00:00
Owner
Copy link

what do you mean "everything undefined"? what's everything? this (with bad endpoint path) sounds like a BE bug, and we should not fix BE's bugs on FE, at least in most cases.

what do you mean "**everything** undefined"? what's **everything**? this (with bad endpoint path) sounds like a BE bug, and we should not fix BE's bugs on FE, at least in most cases.
wyatt777 commented 2019-07-11 13:37:46 +00:00
Author
Member
Copy link

ok. Will reset it back to 10em.

ok. Will reset it back to 10em.
wyatt777 commented 2019-07-11 13:39:47 +00:00
Author
Member
Copy link

It means I get an object back where all of the fields within that object are not defined. i.e. user_id: undefined. Yes, it would be better if the endpoint returned the same way that submitAvatar returns.

It means I get an object back where all of the fields within that object are not defined. i.e. user_id: undefined. Yes, it would be better if the endpoint returned the same way that submitAvatar returns.
wyatt777 commented 2019-07-11 13:43:05 +00:00
Author
Member
Copy link

I do not think you are understanding this one. We have a component user_avatar when I change the code to work properly on undefined, it works. However, the image sample within the settings is not defined within user_avatar. It is set by the state profile_image_url. Image in the component will change for user_avatar, but the image sample within settings will not. This tells me it is more straightforward to set the profile_image_url; this is a state and that is what state's are meant to do.

I do not think you are understanding this one. We have a component user_avatar when I change the code to work properly on undefined, it works. However, the image sample within the settings is not defined within user_avatar. It is set by the state profile_image_url. Image in the component will change for user_avatar, but the image sample within settings will not. This tells me it is more straightforward to set the profile_image_url; this is a state and that is what state's are meant to do.
wyatt777 commented 2019-07-11 13:45:14 +00:00
Author
Member
Copy link

Actually I see what you want now. All locations where these images might occur you want me to make sure they can handle the case that they are undefined? This also seems a bit out of scope because they might occur in multiple components, but would be happy to do it.

Actually I see what you want now. All locations where these images might occur you want me to make sure they can handle the case that they are undefined? This also seems a bit out of scope because they might occur in multiple components, but would be happy to do it.
wyatt777 commented 2019-07-11 14:07:40 +00:00
Author
Member
Copy link

At 12em
Image_2019-07-11_at_11.03.58_PM

At 10em
Image_2019-07-11_at_11.05.20_PM

The only side effect is that min-width for button on settings get slightly longer, this means that the 'submit' button gets slightly longer. My wife says I might be A-blood type because I prefer that the button sizes match each other 😄 Reverting this because it seems out of scope for this task.

At 12em ![Image_2019-07-11_at_11.03.58_PM](/attachments/b48887fb-90f4-46b2-a02b-fb31153dab30) At 10em ![Image_2019-07-11_at_11.05.20_PM](/attachments/c92481a3-0180-4a3e-9ee9-f238379f1bee) The only side effect is that min-width for button on settings get slightly longer, this means that the 'submit' button gets slightly longer. My wife says I might be A-blood type because I prefer that the button sizes match each other :smile: Reverting this because it seems out of scope for this task.
Image_2019-07-11_at_11.03.58_PM.png
162 KiB
Image_2019-07-11_at_11.05.20_PM.png
122 KiB
hj commented 2019-07-11 14:28:33 +00:00
Owner
Copy link

report this to BE then, and while they're at it they should probably change to /api/pleroma endpoints instead of /api/v1.

on topic of "clones" - as shp mentioned - this isn't a clone - you're just mutating same object. I'm not sure you even need to update users in this case. calling it a clone is a misnomer.

report this to BE then, and while they're at it they should probably change to `/api/pleroma` endpoints instead of `/api/v1`. on topic of "clones" - as shp mentioned - this isn't a clone - you're just mutating same object. I'm not sure you even need to update users in this case. calling it a clone is a misnomer.
hj commented 2019-07-11 14:31:05 +00:00
Owner
Copy link

profile banner is used in user card only. user avatar has its own component. profile background is used in one place only, maybe two. I don't think it's that big of a task and it's closely related to this one.

Although, I really do wonder how users what users without an avatar are represented, i think they might have the default string instead of an undefined.

profile banner is used in user card only. user avatar has its own component. profile background is used in one place only, maybe two. I don't think it's that big of a task and it's closely related to this one. Although, I really do wonder how users what users without an avatar are represented, i think they might have the default string instead of an undefined.
hj commented 2019-07-11 14:31:21 +00:00
Owner
Copy link

why are they in /api/v1 tho?

why are they in /api/v1 tho?
wyatt777 commented 2019-07-11 14:59:24 +00:00
Author
Member
Copy link

That sounds correct. 1 to 2 locations for each. Will do that and push it.,

That sounds correct. 1 to 2 locations for each. Will do that and push it.,
wyatt777 commented 2019-07-11 15:01:53 +00:00
Author
Member
Copy link

Do need to update user state or set images to undefined. But agree on everything else. Will look into the changes for this.

Do need to update user state or set images to undefined. But agree on everything else. Will look into the changes for this.
feld commented 2019-07-11 22:14:11 +00:00
Owner
Copy link

Because it's an extension to the Mastodon API. We're keeping it in that namespace and Mastodon can follow our lead or not, doesn't matter to us. This is Marlboro country now :cowboy:

edit: not that different from adding the bookmarks stuff into the Mastodon API namespace but upstream Mastodon doesn't have it either

Because it's an extension to the Mastodon API. We're keeping it in that namespace and Mastodon can follow our lead or not, doesn't matter to us. This is Marlboro country now :cowboy: edit: not that different from adding the bookmarks stuff into the Mastodon API namespace but upstream Mastodon doesn't have it either
hj commented 2019-07-11 22:17:12 +00:00
Owner
Copy link

we had other extensions to mastodon api and they reside in /api/pleroma why this one different?

what are you going to do when they actually implement those endpoint but differently? huh?

we had other extensions to mastodon api and they reside in `/api/pleroma` why this one different? what are you going to do when they actually implement those endpoint but differently? huh?
wyatt777 commented 2019-07-12 01:49:07 +00:00
Author
Member
Copy link

These images can now handle null inputs.

These images can now handle null inputs.
wyatt777 commented 2019-07-12 01:55:21 +00:00
Author
Member
Copy link

Removed the name clone. Still updates the user state to set images to null. Mentioned the empty user object on the original BE issue.

Removed the name clone. Still updates the user state to set images to null. Mentioned the empty user object on the original BE issue.
rinpatch commented 2019-07-12 09:49:19 +00:00
Member
Copy link

I agree with hj, all the previous api extensions were under /api/pleroma or /api/v1/pleroma if they used mastoapi entities, there is no reason this one should be different

I agree with hj, all the previous api extensions were under `/api/pleroma` or `/api/v1/pleroma` if they used mastoapi entities, there is no reason this one should be different
hj commented 2019-07-12 11:23:40 +00:00
Owner
Copy link

i feel like those could be made into a computed property

i feel like those could be made into a computed property
feld commented 2019-07-12 15:56:08 +00:00
Owner
Copy link

Are we always going to be scared of what Mastodon may or may not do or are we going to start moving the API forward ourselves and make Mastodon implement our features?

This should have been brought up before it was merged, but we can change it.

Are we always going to be scared of what Mastodon may or may not do or are we going to start moving the API forward ourselves and make Mastodon implement *our* features? This should have been brought up before it was merged, but we can change it.
kaniini commented 2019-07-12 16:02:18 +00:00
Member
Copy link

I think we should namespace our extensions. Since I merged this the other day, we can simply change the namespacing.

I think we should namespace our extensions. Since I merged this the other day, we can simply change the namespacing.
hj commented 2019-07-12 16:05:07 +00:00
Owner
Copy link

@feld it's not about being scared, it's about being careful at what we do and how we do. They might implement """"""""our"""""""" features different on same or slightly different endpoints, with different behaviors, and then BE (and FE) will have to change it because we also want compatibility with apps.

And hey, since we are on topic of "make Mastodon implement our features" let's go back to QvitterAPI, it's so much better than MastodonAPI and just expect that Mastodon will support that because reasons

@feld it's not about being scared, it's about being careful at what we do and how we do. They might implement """"""""our"""""""" features different on same or slightly different endpoints, with different behaviors, and then BE (and FE) will have to change it because we also want compatibility with apps. And hey, since we are on topic of "make Mastodon implement our features" let's go back to QvitterAPI, it's so much better than MastodonAPI and just expect that Mastodon will support that because reasons
feld commented 2019-07-12 16:09:53 +00:00
Owner
Copy link

There are few clients except PleromaFE that implemented that API, which is the problem. Mastodon's client API is the only client API with adoption which is why we added it in the first place.

There are few clients except PleromaFE that implemented that API, which is the problem. Mastodon's client API is the only client API with adoption which is why we added it in the first place.
hj commented 2019-07-12 17:55:32 +00:00
Owner
Copy link

i take it those "few" clients are again, the ones that (((we))) "control".

this again just an example of not thinking ahead enough. No idea why it wasn't brought up, i remember people being very uncooperative in regards of "extending the API"

i take it those "few" clients are again, the ones that (((we))) "control". this again just an example of not thinking ahead enough. No idea why it wasn't brought up, i remember people being very uncooperative in regards of "extending the API"
wyatt777 commented 2019-07-15 01:24:17 +00:00
Author
Member
Copy link

Possibly. Currently the user presses a button and a confirmation window appears. Interactions like button clicks are normally methods in Vue.

More details?

Possibly. Currently the user presses a button and a confirmation window appears. Interactions like button clicks are normally methods in Vue. More details?
wyatt777 commented 2019-07-15 01:31:01 +00:00
Author
Member
Copy link

Changed API names to /api/v1/pleroma to match backend.

Changed API names to `/api/v1/pleroma` to match backend.
hj commented 2019-07-15 05:49:13 +00:00
Owner
Copy link

i'm talking about avatarImgSrc (src) and bannerImgSrc (src)

i'm talking about `avatarImgSrc (src)` and `bannerImgSrc (src)`
hj commented 2019-07-15 06:01:23 +00:00
Owner
Copy link

those are used to fallback to default when user's avatar is undefined, could easily become computed properties:

  bannerImg () {
    const original = user.profile_image_url_original
    return original || this.defaultBanner
  }
those are used to fallback to default when user's avatar is `undefined`, could easily become computed properties: ``` bannerImg () { const original = user.profile_image_url_original return original || this.defaultBanner } ```
wyatt777 commented 2019-07-15 23:49:59 +00:00
Author
Member
Copy link

That makes sense. Changes made 😃

That makes sense. Changes made :smiley:
shpuld commented 2019-07-26 13:23:36 +00:00
Owner
Copy link

if you use a template literal it's more clear that it's a string we're dealing with

if you use a template literal it's more clear that it's a string we're dealing with
shpuld commented 2019-07-26 13:29:22 +00:00
Owner
Copy link

why do we have the argument if no one ever uses it?

why do we have the argument if no one ever uses it?
wyatt777 commented 2019-07-26 16:22:49 +00:00
Author
Member
Copy link

changed

changed
wyatt777 commented 2019-07-26 16:23:13 +00:00
Author
Member
Copy link

removed! :)

removed! :)
kaniini commented 2019-11-08 02:18:35 +00:00
Member
Copy link

Has the BE issue been resolved? Is there an issue I can look at? I tried searching and didn't find anything that seemed relevant.

Has the BE issue been resolved? Is there an issue I can look at? I tried searching and didn't find anything that seemed relevant.
wyatt777 commented 2019-11-15 03:11:11 +00:00
Author
Member
Copy link

yes, I am rebasing to get this pushed.

yes, I am rebasing to get this pushed.
wyatt777 commented 2019-11-15 06:01:19 +00:00
Author
Member
Copy link

@shpuld I rebased this for review. In the test the Firefox browser keeps failing. Any idea why that may happen?

@shpuld I rebased this for review. In the test the Firefox browser keeps failing. Any idea why that may happen?
hj commented 2019-11-15 08:51:10 +00:00
Owner
Copy link

CI is borked rn

CI is borked rn
kphrx commented 2019-11-16 07:59:24 +00:00
Member
Copy link

It may be solved if shm size set 2g
https://github.com/karma-runner/karma-firefox-launcher/issues/104#issuecomment-522371253

For example, setting DOCKER_SHM_SIZE in Settings -> CI / CD -> Variables?

It may be solved if shm size set 2g\ https://github.com/karma-runner/karma-firefox-launcher/issues/104#issuecomment-522371253 For example, setting `DOCKER_SHM_SIZE` in Settings -> CI / CD -> Variables?
wyatt777 commented 2019-11-18 23:29:44 +00:00
Author
Member
Copy link

Thanks!
In any case, this is ready for review @hj @shpuld .

Thanks! In any case, this is ready for review @hj @shpuld .
wyatt777 commented 2019-12-12 14:20:38 +00:00
Author
Member
Copy link

@shpuld @hj this one is still ready for checking :)

@shpuld @hj this one is still ready for checking :)
hakabahitoyo commented 2020-01-17 20:21:45 +00:00
Member
Copy link

This line seems to break the Who to follow panel.

This line seems to break the _Who to follow_ panel.
hakabahitoyo commented 2020-01-17 21:17:50 +00:00
Member
Copy link

This works well: hakabahitoyo/pleroma-fe@269304d3c2.

This works well: https://git.pleroma.social/hakabahitoyo/pleroma-fe/commit/269304d3c28e096d45ac4a490bf2657c4e14cacc.
wyatt777 commented 2020-01-18 08:02:42 +00:00
Author
Member
Copy link

I need to resolve the merge conflicts and will change that. Thanks!

I need to resolve the merge conflicts and will change that. Thanks!
shpuld commented 2020-06-27 10:22:29 +00:00
Owner
Copy link

I'm branching off of this branch to fix the conflicts and to make some things look prettier at #2423

I'm branching off of this branch to fix the conflicts and to make some things look prettier at #2423

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
8 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!2146
No description provided.