#6570 closed enhancement (fixed)
Add UI for adding Profile Header Images for Users and Groups
Reported by: | mercime | Owned by: | |
---|---|---|---|
Milestone: | 2.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Core | Keywords: | dev-feedback has-patch commit |
Cc: | hugoashmore@… |
Description
aka Cover Photos. This feature was discussed in dev chat before and I was one of those who mentioned that there was a plugin in the WP repo. However, since we have a great Attachments API and have tested it well, I suggest that we include this feature in core to make it so much easier for users to activate and use the feature if they want it in their sites.
Attachments (10)
Change History (84)
#3
@
9 years ago
Nice @modemlooper, it would be awesome if you could suggest a patch using the Attachments API :)
I think we've talked about this in dev-chat during 2.3 cycle as a possible use for the BP Attachments API, i see 3 good reasons to have this in core :
- profile headers are public files
- every social network have one (g+/twitter...)
- Mercime and I like this feature :)
Other than the fact there are some existing plugins providing the feature, is there any reason i don't see that could explain not to have this ?
#4
follow-ups:
↓ 6
↓ 16
@
9 years ago
We should talk to the @buddyboss folks and see how they've handled this. Maybe there's opportunity to learn from their experience and adapt it to core.
I also think it's a good idea for the functionality to be built into the core component, but that the bp-default
theme compatibility should register the dimensions or aspect ratio. Since the theme controls the header size but core facilitates the upload & provides the UI for cropping, I think it's important that responsibilities be separated, and respectful of each other's expectations. Does that make sense?
#5
follow-up:
↓ 8
@
9 years ago
As much as this is a nice feature on some sites, I don't think it should be a core thing. This should be for themes only. and this seems a step back before template compatibility
#6
in reply to:
↑ 4
;
follow-up:
↓ 7
@
9 years ago
Replying to johnjamesjacoby:
We should talk to the @buddyboss folks and see how they've handled this. Maybe there's opportunity to learn from their experience and adapt it to core.
oh, completely ignore my code, eh? I copied the avatar code and modified it for cover images.
#7
in reply to:
↑ 6
;
follow-up:
↓ 9
@
9 years ago
Replying to modemlooper:
oh, completely ignore my code, eh? I copied the avatar code and modified it for cover images.
Your code isn't here to ignore.
#8
in reply to:
↑ 5
@
9 years ago
- Keywords dev-feedback added
Replying to modemlooper:
i kind of "not agree at 100%" with :
As much as this is a nice feature on some sites, I don't think it should be a core thing. This should be for themes only. and this seems a step back before template compatibility
Earlier you've said you built 2 plugins to do so ? So it's a theme feature that you packaged into plugins ?
I really think we should include something in core about this feature but do it the right way. If this is a theme feature, then we can rely on something like add_theme_support( 'buddypress_single_item_header' );
to leave the choice to the theme to support or not the feature.
As @johnjamesjacoby said : then, we should provide an API that is easy for theme designers to use and customize. From the bp-legacy template pack we'll then be able to show how to use this API (dimensions...).
btw guess who's coming to BuddyCamp Brighton : https://brighton.buddycamp.org/2015/speakers/ :) I'll try to talk with Michael about it!
My main concern is more on how to organize the BuddyPress uploads. Because so far we're creating a specific folder for each feature directly under uploads
e.g.: group-avatars
I think we should begin to think about the best way to organize this part.. For instance:
uploads/ buddypress/ members/ 1/ avatar/ cover/ 2015/ 07/ groups/ 3/ avatar/ cover/ 2015/ 06/
etc..
#10
@
9 years ago
I should explain, I don't think we should add a cover photo UI without rethinking avatar upload crop. This process should be generic. You should be able to upload and crop various size images and then use images for various purposes. BuddyPress should provide base functionality without forcing a usage if possible.
#11
@
9 years ago
I agree at 100% !
I also think it's hard to do half of the path without generating some frustration for some users.
"basic" i doubt we can stay too basic :)
I think we must stay very customizable/overridable. The cover photo is an interesting feature because it can help us really improve the BP Attachments API so that we have a library to really attach any kind of "public" file to any kind of BuddyPress objects.
But we need to provide the user a way to manage these files (remove/set as avatar/set as cover...)
Using some filters/actions it's already possible to use the BP Attachments API to do this, but it really should be converted into a more "visible" feature imho. The cover photo can help us do that :)
#12
follow-up:
↓ 13
@
9 years ago
@modemlooper btw you should use the BP Attachments API in your cover image plugin :)
#13
in reply to:
↑ 12
@
9 years ago
Replying to imath:
@modemlooper btw you should use the BP Attachments API in your cover image plugin :)
We wrote this plugin at WDS before the api was in BP
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#16
in reply to:
↑ 4
@
9 years ago
Replying to johnjamesjacoby:
We should talk to the @buddyboss folks and see how they've handled this. Maybe there's opportunity to learn from their experience and adapt it to core.
We're using plupload. It's worked out nicely, works on phones and is pretty compatible across browsers. Although currently we can't do cropping like you have with BP Attachments API. We've considered switching to use BP Attachments API, but it would be problematic when someone disables BuddyPress. As a theme it needs to work with or without BP.
If you're adding it to core, yeah it should probably be done using Attachments API. Then maybe we could disable the cover photo if BP is disabled, and then hook into BP's native cover photo functionality with BP enabled.
@modemlooper Your mockup to drag the handles is great, that is how users wish it worked. We store the image taller than we need and crop it vertically within the div, so when the browser goes mobile and content necessarily gets taller, there is still more image left to display. This is something to consider, otherwise text will spill out of the image on mobile layouts.
#17
@
9 years ago
my plugin doesnt break if you deactivate BP. it is basically a copy of avatar upload and crop. does not use attachments API because we created it before that.
#19
@
9 years ago
I'm sorry, this is going to be a long comment :(
First i now understand better r-a-y's advice about the fact we should only provide support for "twenties" theme. I think he's absolutely right.
Second, it's definitely a theme feature, so i really think we should rely on the WordPress "theme_support" API.
Third, thanks for your feedback @BuddyBoss, it made me realize that if the theme add supports for the cover image then when checking if the current theme supports the feature within a plugin or the theme, it will return true even if BuddyPress is deactivated, so if you need to include file that are using BuddyPress functions or classes, i guess we need to find an easy way to try to avoid any trouble.
So in 6570.first-step.patch, i've taken in account these three points. And here's what's inside this first step:
Here's an example of how a theme could set the support for BuddyPress cover images :
add_theme_support( 'buddypress-cover-images', array( 'width' => $width, 'height' => $height, 'callback' => 'theme_prefix_cover_images_callback', ) );
Imho, as it will be very difficult for BuddyPress to be sure of how the item-header is for any theme, i strongly advise the theme to define the callback parameter. This callback will then return an inline css we will be able to output.
Then i thought this may be enough in most cases, but maybe not in some cases. So i think we should also be able to have different parameters according to the single item. So far we have Members and Groups single items, and in a near future the Blogs one and who knows plugins can also add their specific single items..
So i think we should extend this by component. In the patch bp_set_theme_support()
is doing this job. If the theme support is like the above, each single items header will use these parameters, but a theme might want to use different dimensions/callback according to the component. So it can achieve this by using :
add_theme_support( 'buddypress-cover-images', array( /* buddypress()->members->id */ 'members' => array( 'width' => $width_members, 'height' => $height_members, 'callback' => 'theme_prefix_members_cover_images_callback', ), /* buddypress()->groups->id */ 'groups' => array( 'width' => $width_groups, 'height' => $height_groups, 'callback' => 'theme_prefix_groups_cover_images_callback', ), ) );
Or the theme can also choose to support only one component, for example to only add support to members cover images, he can do this :
add_theme_support( 'buddypress-cover-images', array( /* buddypress()->members->id */ 'members' => array( 'width' => $width_members, 'height' => $height_members, 'callback' => 'theme_prefix_members_cover_images_callback', ), ) );
I've also made available a 'default_cover' parameter if the theme wants to provide a "mystery cover". I think BuddyPress just need to display the item-header like it is today if the user/group has no cover image.
If the theme's need to be sure BuddyPress is ready to bring the support, he can use this check :
if ( ! current_theme_supports( 'buddypress-cover-images', 'inactive' ) ) { require( 'file' ); }
By default without BuddyPress activated, current_theme_supports( 'feature', 'parameter' )
will always be true if the theme added support for the feature. bp_set_theme_support()
will set inactive as false, meaning BuddyPress can bring the support. Of course the theme could also use something like:
if ( function_exists( 'buddypress' ) && bp_is_active( 'members' ) ) { require( 'file' ); }
But i thought it was less pretty :)
About the twenties. BP_Legacy->setup_supports()
is there to bring the BuddyPress cover images feature by default (unless the twenty theme already added this support, eg: a child theme could do this). I've realized that since the introduction of the companion stylesheets that for each concerned themes we cannot rely anymore on the $content_width
global. So i've used width my browser calculated for the themes having a companion stylesheet, i guess it needs to be checked..
Now how the cover image should look like ?
First i've tested @modemlooper's plugin. And unless i did something wrong, once i've uploaded a valid cover image, nothing was displayed into my profile header. After a deeper look, i guess i may miss a css rule in my theme, because the plugin is adding a new div above the item-header and uses the uploaded cover photo as a background. The problem is my div has no height, and the cover photo is not displayed. I've added a height of around 300px and it showed over the item-header. Imho, i think we shouldn't add an extra div but directly use the existing #item-header
a bit like what's doing the Boss theme by @BuddyBoss. But then, we have a probleme! We do have a lot of text in this div, and once i've tested without too much css manipulation, this is what i got on twentyfifteen:
It's pretty difficult to read the text!
I like what's doing the Boss theme, well Facebook & Twitter are kind of doing similar things, they use a white color with a text shadow. But applying this trick on each text of this div without touching the action buttons is a bit annoying and we hardly enjoy the cover image as she's full of text. So the best way of displaying it seems to be this one imho:
Even if twentytwelve fails (because unlike the others the item-header-content has no width) It's interesting to see that the white on white is still readable :)
The next problem is smaller screen. If we're ok with the layout i'm suggesting, i think we should have this kind of layout on smaller screens :
As you can see this is not an easy part, and i'm not the best in web design! So it would be awesome if @hnla @mercime @BuddyBoss and every "web design star" could find the best solution on how to display this cover photo. If you need to run some tests, you just need to apply the patch and switch themes. The css part to edit is in src/bp-templates/bp-legacy/buddypress-functions.php
inside the bp_legacy_theme_cover_image()
function. PS: i haven't done anything for the groups cover images as you will see.
Uploads / BP Attachments API
I'm now going to explore this part of the patch, but before going too fast and furious about it, i'd like to have your feedback about my first ideas:
- the UI to set the cover image should be similar to the Avatar one (extendable tabbed interface: Upload|delete at the begining)
- It's time we think about what's the best file organization. as i've said in this comment. My personal preference would be
uploads/buddypress/component_id/component_single_item_id/subfolders
where subfolders could be 'avatar', 'cover-image', 'year/month' (< like WordPress Year than a subfolder for each month). For the members component it would beuploads/buddypress/members/1/cover-image
. Problem is:rename( 'uploads/avatars/1', 'uploads/buddypress/members/1/avatar' )
. Of course we can use/uploads/cover-images
,/uploads/group-cover-images
etc.. But it seems less "tidied" to me, and i think having a 'buddypress' root folder makes it easier to understand for users.. - It may be the time to "init" what could be the structure of a future required component using a specific custom post type to organize the files and extending the WordPress Attachments API according to our specific needs.
- We would then be able to have a very generic upload UI and eventually add a new browse 'tab' to the avatar's one and the future cover images's one so that users could be able to choose an already uploaded image as an avatar or a cover photo, or so that it would be easier to include an 'Upload|Browse' UI inside the activity post form...
Or we could reproduce how we're doing with avatars..
What do you think/prefer ?
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#21
follow-up:
↓ 22
@
9 years ago
I 100% disagree we should only support core theme's and if this is the case it should never go into BuddyPress core. We need to be theme compatible. If you uploaded a plugin to the repo and it only worked with specific themes it would be rejected. This is a step backwards for theme compatibility. We supply the BP templates and can make a header work within the confines of those template files. The parent theme has no CSS for BuddyPress templates unless it has extra styling and all BuddyPress UI and functionality works on any theme.
#22
in reply to:
↑ 21
@
9 years ago
Replying to modemlooper:
This is actually not exactly what i've tried to explain...
I 100% disagree we should only support core theme's
Any theme can use the feature as long as they wish to by adding support for it within their functions.php file using one of the code i've shared above.
Twenties only get a default support and don't need to add it within their functions.php
Honestly it would be really difficult to deal with any theme unless if we provide a functionality like your plugin does eg: put an image over the header with no extra css.
#23
@
9 years ago
And btw if we were to be strict, using add_theme_supports() means it is to the theme to decide whether to enable the feature or not we shouldn't even do it for twenties ;)
#24
@
9 years ago
I feel strongly against using add_theme_support. If we build this as a core feature that themes can support, either they support it or they don't; no additional declaration required. And bp-legacy shouldn't need to declare support for anything since it should come with everything.
Imath, your screenshots look basically perfect IMO. If you've got it looking and working well in all of those themes, that's ideal.
In general, it seems most cover photo UI's do not include cropping, and instead include a placement mechanism to center or align an image, so that it can flex along with changes in height and width. If we crop it to explicit dimensions, we risk exposing an empty background area. I suppose we can crop & compress for compliance sake inside of our API, but I haven't seen a cover photo UI that cropped, and I have a hunch that responsive design and being flexible is the reason.
Imath, can you help me better understand why this will not work without add_theme_support? It's possible I don't understand what problem it solves for us, but I've always thought of it as a backwards and generally useless API that promotes lock-in vs. providing compatibility and flexibility.
#25
@
9 years ago
Thanks johnjamesjacoby for your feedback.
I think i forgot the most important in my previous comment : "Why should we use/extend add_theme_support() ?"
I'm sorry about this. I understand your point of view and the one of modemlooper about it and the philosophy behind that everything should be compatible with any theme. BP Theme Compat is great and i wasn't trying to move backward.
Maybe i was influenced by all the feedbacks about "this is a theme feature.." and this particular part during our last dev-chat.
Even if in the case of the cover image i am convinced we should use add_theme_support()
to have a safer feature, i will not insist too much about it :)
If the goal with this feature is to basically put an image inside the #item-header by injecting a new div so that we are sure to find it. I'm ok that we don't need add_theme_support()
we can simply add some uploads functionality with a very large image and we're done. But this won't be great, this will look like this :
https://cldup.com/-1-bLUNEXO.png
So anyway, this basic feature will require people to adapt their css because, i'm sorry but this is terribly ugly :)
If the goal is to have a nice cover image feature that any theme can implement & control the best way, then we should be there to provide them all the help in the upload (/crop if needed) process part and leave them the maximum freedom to make it look awesome into their themes. In a way be in the API part and leave them control the layout part.
People may have overriden templates, changed div ids, themes can want a specific height x width, put some white text in cover image, move the avatar.. (a bit like i did in my screenshots), that's a lot of unknown things... I think the best way to achieve it is by injecting some css dynamically thanks to the callback part that themes will be able to completely control according to their layout, content width, screen sizes ....
That's what the patch was suggesting. I did nothing about uploads because i'd like first to have your feedbacks about all the ideas i have in mind and that i've shared in the end of my previous long comment :)
Again, i think it's the right/safest move but i can be wrong, and if you think i am, then i'll do something very basic and will concentrate on the most interesting thing to me > The Attachments API/UI part :)
#26
@
9 years ago
Thanks imath as always for your quick turnarounds :)
This is pretty much what I had in mind. A pity that WordPress defaults to true
for current_theme_supports()
.
Replying to modemlooper:
I 100% disagree we should only support core theme's and if this is the case it should never go into BuddyPress core.
Replying to johnjamesjacoby:
I feel strongly against using add_theme_support. If we build this as a core feature that themes can support, either they support it or they don't; no additional declaration required. And bp-legacy shouldn't need to declare support for anything since it should come with everything.
imath's replies (comment:22 and comment:25) are exactly why I mention that we should only add support on themes that we have companion stylesheets for.
We need to be able to set the cover photo width and height for cropping and displaying. This cannot universally be done properly. We could set the width based on the theme's $content_width
if available, but not every theme offers this.
The alternative is to offer a "tamer" version of the cover photo that works on every theme, which I do not think people will like as a default feature.
However, if we can get the markup to a point where the cover photo looks consistent without any weird design quirks, then we can think about rolling with it.
#27
@
9 years ago
I think we should provide support for all themes but let admins enable the feature for users and/groups themselves per my screenshot posted above.
Perhaps add a line under the "Allow registered members to upload cover photos" for example, "Check if your theme is compatible with this new feature <link to codex article>". That way, we avoid conflicts with existing sites which have a cover photo plugin activated or sites which use a theme that has incorporated this feature already.
There are 100,000+ sites with BuddyPress activated, I'll bet that the majority either have not touched the BP legacy template files nor have premium themes which have changed the HTML markup for item header.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#29
@
9 years ago
Ok so the plan is to create a new template for the has_cover_image
header and edit the parent template home.php
, this way premium/custom themes will be able to have time to make sure the feature is looking good into their themes before editing home.php
and adding the new template.
Then @mercime is right, we need to think about existing plugins, themes having a cover photo feature. Even if adding new options is a bit annoying, these do have an interest if not activated by default with an explanation to explain why it's not activated by default.
On the other hand, when we add a new feature it's activated by default... To be honest i'm hesitating with another possibility: use and make the avatar options more generic 'allow user uploads / allow user uploads for groups' and use bp_is_active( 'component', 'feature' )
.
#30
@
9 years ago
@imath
Agreed, cover photos should not be activated by default. When people update BuddyPress they expect feature updates, as it's a plugin, and not to have the layout of their site suddenly changed. Cover photos are a major design element to add to a profile and will look weird on a lot of themes without adjustments by the theme author.
In our themes for example, we will need to add a condition to use the native has_cover_image functionality if running latest BP, and to default to our own cover photo uploader if using a previous version of BP. Otherwise if they enable it, it will break the whole layout immediately. Lots of other layouts will get broken too.
This leads to a bigger concern, that BP determines a lot of layout for a plugin, the end result being that every BP site looks almost the same. The more we can prevent that problem the better.
We need to be able to set the cover photo width and height for cropping and displaying.
Absolutely. The theme author needs to be able to set width and height. Some will want a small image, and others will want it huge, in the style of medium.com. And many authors will want to overlay the avatar and username on the photo, similar to FB/Google+, and so we'll need a taller image that can expand vertically on mobile to accommodate everything. Basically, you can't predict what people will do with this so it needs to be adjustable. It's a visual element.
#31
@
9 years ago
Thanks a lot @BuddyBoss for your very interesting feedback.
In latest devchat we agreed to not use the Theme Support API because we've found a way to play nicely with Standalone BuddyPress Themes.
In short when the feature will be there it won't change anything for themes having their own BuddyPress templates, to be more precise if the theme is overriding groups/single/home.php
and members/single/home.php
then nothing will happen.
Same for the 'change cover image' template, it will be a new one that will be loaded by members/single/profile.php
so if you have overridden this template, nothing will happen. For groups we will need to edit the groups/single/admin.php
template.
What's i'm working on now is to find a way to add the new navigation into the BuddyPress Nav or the Admin Bar only if the current theme is using bp-legacy
.
Then how to easily allow to customize h x w and finally Uploads :)
I'll keep this ticket updated as soon as i'm making some significative progress.
About
This leads to a bigger concern, that BP determines a lot of layout for a plugin, the end result being that every BP site looks almost the same. The more we can prevent that problem the better.
I understand the concern, but that's a great thing to have it when the theme is not a BuddyPress theme. I think: it would be great to have different template packs anyone could activate according to their layout preferences :)
In the meantime i think we can work on making it easier to override/extend...
#32
@
9 years ago
Thanks @imath for the description. This makes sense and appears to be an elegant solution.
#33
@
9 years ago
- Keywords has-patch added
Again, get ready for a pretty long comment!
So i've been seriously working (not to say "fast & furiously"!) on a new patch: 6570.02.patch. I focused on the User's profile header, but plan to carry on if we all agree on the choices i've made :)
But before, if you wish to see the feature in action, here's a demo of it: https://vimeo.com/136356743
BP Theme Compat instead of add_theme_support()
We made this choice during latest dev-chat. In short: we are editing some BP Legacy templates so that any BuddyPress standalone themes will have the time/choice to edit their templates to also use it. In the meantime we make sure to play nice with their layout. Any WordPress Theme using BP Theme Compat (as soon the templates we're editing are not overriden in them) can use the feature immediately.
The edited templates in the patch are :
src/bp-templates/bp-legacy/buddypress/members/single/home.php
: it now includes a conditional check to see if the displayed user has a cover image, if so we load the new header templatesrc/bp-templates/bp-legacy/buddypress/members/single/cover-image-header.php
else, we load the regular header template.src/bp-templates/bp-legacy/buddypress/members/single/profile.php
: it now includes a check on the current action to eventually loadsrc/bp-templates/bp-legacy/buddypress/members/single/profile/change-cover-image.php
and be able to set the user's cover image.
BP User Navigation & WP Admin Bar loggedin user's menu
To make sure no 'Change your Cover' item nav is added in case the BuddyPress standalone theme has not implemented the template changes, this is my suggestion: use bp_is_active( 'component', 'feature' )
and introduce a new function bp_set_theme_compat_feature()
. This way nothing's happening for the themes i was talking about, and for Themes using BP Legacy, the feature is automatically registered within src/bp-templates/bp-legacy/buddypress/buddypress-functions.php
. bp_set_theme_compat_feature()
is doing 2 things:
- carry the feature settings (components, width, height, callback..)
- activate the feature for the targetted components, in the case of the 'cover_image' feature it's
xprofile
andgroups
.
Then before adding the nav, i'm simply doing a bp_is_active( 'xprofile', 'cover_image' )
so that we are completely playing nice with Standalone Themes.
About Options: @mercime suggested to add new options to let Admins disable/enable the feature. To be honest i'm feeling pretty incomfortable about adding new options. But of course i can be wrong and it will always be possible to add these options later.
My personal opinion is: as an admin, i would allow users to upload files or groups to upload files no matter if it's an avatar or a cover image or any other features. And for the particular case of the cover image, without an avatar it's not great :) So if avatar uploads are disabled, i disable the cover image.
Moreover, in the future, there's a good chance we add other attachments features, i'm not sure it's great to systematically add a new option for each new feature. My suggested plan about it, is:
- 1 option by component eg: allow user uploads, allow group uploads
- then a more granular control using for instance
add_filter( 'bp_is_active_xprofile_cover_image', '__return_false' );
Companion Stylesheets & BuddyPress Cover Image's inline css
We now have companion stylesheets to improve BuddyPress layout for twenties. 3 themes so far are concerned :
- twentythirteen,
- twentyfifteen,
- twentyfourteen,
The companion stylesheets are a great improvement, but they are not necessarly taking in account the $content_width
global of the theme. So for these themes, it's pretty difficult to figure out the #buddypress
width, which in the case of cover image is kind of important!
So you'll see that in the new BP_Legacy->setup_features()
, i'm using by default $content_width
which works great with twentytwelve for instance (and i guess with most themes), and then use specific $bp_content_widths
for the themes having a companion stylesheet. It works fine with 2 of them, but twentyfifteen is problematic, so i'm using an arbitrary value as it's impossible to know about any max-width as the BuddyPress content takes all the available space left at the right of the sidebar. Ideally, it would be great for this particular theme (at least) to define a #buddypress { max-width:1300px; }
for instance.
In src/bp-templates/bp-legacy/buddypress/buddypress-functions.php
, you'll find the callback function bp_legacy_theme_cover_image()
that returns the custom css for the cover image header. I've included a lot of rules into it so that this specific header looks similar in twenties. But i think this would need to be improved. Most of the css rules should probably be in buddypress.css for some and in companion stylesheets for others. I think in this function we should only have the 2 first rules.
The new cover-image-header.php
I've reorganized the markup a bit, so these changes surely need to be checked. Basically, i'm adding a container: div#item-header-cover-image
, the cover image is the background of this selector.
The new BP_Attachment_Cover_Image class & the Cover Image UI
The class is managing the uploads and resizing images so that it fits with the cover image settings. I'm not using the cropping tool. @BuddyBoss is not using it either and @johnjamesjacoby was thinking it was not that necessary. I feel the same, and i'll add that now that the User's subnav is vertical in the Themes having a companion stylesheet, the cropping interface is tighter than the needed width which is not great. So i'm dynamically cropping the image.
Upload dir organisation: in a previous comment i already talked about my opinion about this. I think we need to reorganize this part. And I've begun with this feature. Users cover image will be in /wp-content/uploads/buddypress/members/user_id/cover-image
. I think this kind of organisation will allow us to have a very obvious place for any administrator about where we are saving files, and it will allow us to save other files or directories (eg: YY/MM) inside the /wp-content/uploads/buddypress/members/user_id
in future releases.
NB: i'm not touching to avatar folders, but ideally i think they should move into /wp-content/uploads/buddypress/members/user_id/avatar
or /wp-content/uploads/buddypress/groups/group_id/avatar
.
When a user is setting or removing his cover image, the header is dynamically refreshed without any page reload.
Possible improvements: avatar.css is used because i've put the Uploader css rules in there. I know @hnla posted a ticket about this.
BuddyPress standalone Themes.. How to?
For standalone themes wishing to use the BuddyPress Cover Images feature, i've made some tests using the only one i have: "Swifter". Many thanks to @johnjamesjacoby :)
1) How i've registered the Cover Feature
a) the easiest way:
bp_set_theme_compat_feature( 'legacy', array( 'name' => 'cover_image', 'settings' => array( 'components' => array( 'xprofile', 'groups' ), /* or you can only use array( 'xprofile' ) to restrict the cover image to users */ 'width' => 1170, 'height' => 225, 'callback' => 'swifter_theme_cover_image', /* function that will return to BuddyPress the css to attach to the theme_handle */ 'theme_handle' => 'swifter', ), ) );
b) an Alternative way:
add_filter( 'bp_is_active_xprofile_cover_image', '__return_true' ); function swifter_xprofile_cover_image( $settings = array() ) { return array( 'components' => array( 'xprofile' ), 'width' => 1170, 'height' => 225, 'callback' => 'swifter_theme_cover_image', 'theme_handle' => 'swifter', ); } add_filter( 'bp_before_xprofile_cover_image_settings_parse_args, 'swifter_xprofile_cover_image', 10, 1 );
2) The callback function
Then i've copy-pasted the bp_legacy_theme_cover_image()
function into the swifter theme, without forgetting to rename it to match the callback argument in bp_set_theme_compat_feature()
: 'swifter_theme_cover_image'. I've edited the inline css returned by this function to ajust css rules to the Swifter theme.
NB: Then, i had to change the priority Swifter is using to hook wp_enqueue_scripts
as it was too late. Using a 9 priority (just before bp_enqueue_scripts
) is fine. Should BuddyPress hook to wp_enqueue_scripts
at a later priority to avoid this ?
3) Edit the templates
Finally i've edited src/bp-templates/bp-legacy/buddypress/members/single/home.php
and src/bp-templates/bp-legacy/buddypress/members/single/profile.php
to include the changes included in the patch for these two templates. You don't need to add the new templates, the templates of BP Legacy will be used.
Let me know what you think about my suggestions :)
#34
@
9 years ago
Very cool. You may be surprised at how many people do want cropping though, or more specifically, image positioning.
#35
follow-up:
↓ 36
@
9 years ago
Thanks for working quickly on this, imath!
I've made some changes to the cover image template part - 02.markup-changes.patch
.
Patch does the following:
- Moves the cover image into a separate element -
a#header-cover-image
. This is so we can control the cover image separately outside the main avatar and button markup. Cover photo is also clickable using the<a>
element (similar to Facebook).
- Fixes an issue if an admin switches to a theme with a larger width and the cover photos were not taking up the width of the new theme. Cover photo now uses this CSS declaration --
background-size: cover
-- so the image takes up the remainder of the container. I've also altered the CSS so the cover photo looks more consistent across different themes and have done some preliminary CSS work for our companion stylesheets. Companion stylesheets are still a work-in-progress.
- Shows the cover photo markup even if the user has not uploaded a cover yet. I've set the default background color to
#c5c5c5
, which matches the Gravatar default avatar background. This is so profiles without a cover photo will look consistent with profiles that have a cover photo. Also, this might encourage users to set a cover photo! We could set the cover photo link if a user is viewing their own profile so it links directly to the "Change Cover Photo" page.
- Latest member activity update now shows in the right-hand container with the at-mention name and buttons. Didn't really like the member update cleared on its own line. We could even consider removing the latest member activity update from the header entirely for our cover photo template part.
Feedback for comment:33:
bp_set_theme_compat_feature() vs. add_theme_support()
A part of me would still prefer using add_theme_support()
to declare cover photo support, but bp_set_theme_compat_feature()
is pretty cool because devs can use the 'bp_parse_args' filters as in the swifter_xprofile_cover_image()
example above.
File renaming
I'm fine with the proposed filenames for the cover photo. For avatars, I think we should leave them alone for the time being.
One really minor tidbit is for sites that are whitelisting BuddyPress. They might not like wp-content/uploads/buddypress
. It's a small thing that a dev can probably filter, but thought I'd bring it up.
Cropping
As BuddyBoss also has stated, I think people will want to crop their cover photo so it displays the way they want it to.
Admin option
If we enable cover photos by default for every theme, I would agree with mercime that we might need to consider an admin option. Not all site admins love dealing with code snippets to disable something that we force enable by default in a major release.
Btw, this is the snippet to disable profile cover image support:
add_filter( 'bp_is_profile_cover_image_active', '__return_false' );
Should this be 'bp_is_xprofile_cover_image_active'
?
I see some code to force the component from 'xprofile' to 'profile' in bp_set_theme_compat_feature()
. Is there a reason why this is done? Is this because the xprofile component might not be active?
#36
in reply to:
↑ 35
@
9 years ago
Replying to r-a-y:
I've made some changes to the cover image template part -
02.markup-changes.patch
.
Thanks a lot r-a-y. All your "bullet points" changes are really interesting and are great improvements. I have only one problem with :
- Shows the cover photo markup even if the user has not uploaded a cover yet.
My concern is:
I think we should let Admins decide about this. Some might want to have regular header if the user has no cover image and some other might want to "force" user to upload a cover image and i think we should let them free to choose. For example, they could do this to have the grey background :
add_filter( 'bp_displayed_user_has_cover_image', '__return_true' );
bp_set_theme_compat_feature() vs. add_theme_support()
I think bp_set_theme_compat_feature()
is more in the BuddyPress philosophy. As @johnjamesjacoby and @modemlooper said in previous comments or in dev-chat: Feature should be available to any WordPress theme using BP Legacy. The way we add the support for the cover image in buddypress-functions.php
(+ the new templates and the edited templates) is just making sure we play nice with BuddyPress standalone/premium themes.
So even if i was feeling a bit like you about add_theme_support()
, i am now beginning to think we're on the right way with this new function :)
File renaming
One really minor tidbit is for sites that are whitelisting BuddyPress. They might not likewp-content/uploads/buddypress
. It's a small thing that a dev can probably filter, but thought I'd bring it up.
We can choose another name, what about 'bp-uploads' ?
Cropping
As BuddyBoss also has stated, I think people will want to crop their cover photo so it displays the way they want it to.
Well, i really don't feel the cropping tool. It's already a pain for avatars as soon as we are on mobile devices, then for themes having a vertical nav, we don't have all the $content_width
available, so i guess it can be pretty challenging to calculate ratios etc. I may be wrong about it so i'll check how the plugin modemlooper shared earlier in the thread is behaving with twentyfifteen.
Moreover, you took the example of Facebook, Facebook (or Twitter by the way) does not include a cropping tool, you can eventually move the background but not crop it. That's probably because in mobile devices cropping mustn't be that great...
I'd say i'd rather only automatically crop the width and keep all height and do a bit like facebook so that the user can adjust the visible part of his header. But this is meaning adding some js code into cover-image.js and new css rules ;)
Admin option
Well, although a part of me disagree, i don't have any strong opinion about adding new options, so if majority's fine with it, let's do it :)
I see some code to force the component from 'xprofile' to 'profile' in
bp_set_theme_compat_feature()
. Is there a reason why this is done? Is this because the xprofile component might not be active?
That's because bp_is_active() is checking if the component is active using the option bp_get_option( 'bp-active-components' )
And in this option we have like array( 'xprofile' => 1 )
. So xprofile
is used. But when the xProfile component is created it's using buddypress()->profile = new BP_XProfile_Component()
in its loader. So the feature is attached to buddypress()->profile
So 6570.03.patch is including all improvements r-a-y suggested in his patch except for the part that was "showing the cover photo markup even if the user has not uploaded a cover yet"
Then i have a new concern, considering 2.4 dev-cycle and the time we have till beta. Should we focus on the user's cover image and work on the group's one in next release ?
#37
@
9 years ago
todo: Manage cover photo from the Edit user's/profile Administration screen. #6604 can be helpful for this imho.
#38
@
9 years ago
As discussed during latest dev-chat i've been working on a new "user flow" to set the user's avatar and cover image by adding 2 buttons directly inside the header that are using an improved version of the patch i've first posted on #6604.
Thanks to this modal box, i was able to also work on the wp-admin/extended profile to set/delete/edit the user's cover image.
Then, @r-a-y was right gray header! So i have no more objections about it
Then, @r-a-y and @mercime were right about the options to disable/enable the feature being a "regular" user, i've added them.
This made me realize that the bp-templates/bp-legacy/buddypress/buddypress-functions.php
is never loaded inside WP Admin as it's loaded using bp_locate_template()
. Meaning we can't use this file to set the default feature of the template pack (so far only cover image). As a result i've moved it inside /bp-core/bp-core-theme-compatibility.php
.
Finally, i've added the cover image to the Groups component: create/manage/administration screens. Here's a screenshot of it:
So we have a first "full featured" patch. 6570.04.patch also includes 6604.patch so it's easier to test it. And precisely we need now to test it to check everything is fine with all WordPress default themes. I've added filters and actions, but feel free to suggest new ones at key points.
Also, once this gets in, maybe we should think about a new activity action e.g.: "imath changed their cover image"
Oh and one more thing :) Here's a video demonstrating the patch
https://vimeo.com/138356216
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#40
@
9 years ago
Just to note that per tweet last Sept. 4, tested on Twenty Sixteen and it works! :)
#41
@
9 years ago
Hi, 6570.05.patch is improving some parts of 6570.04.patch.
1/ Introduce new functions to get/check allowed extensions, allowed mime types and max upload file size (and edit the avatar ones to use these new functions)
2/ Improve javascripts thanks to grunt jstest
3/ Improve Groups cover image for twentytwelve.
I think it's time for us to test this live :) That's why i've updated the test drive i used for the introduction of the 2.3 Avatar UI with latest trunk + 6570.05.patch.
http://imath-buddypress.wpserveur.net
Feedback are welcome here or on the test drive.
If you wish to test and don't have a login yet, you can contact me using slack, and i'll create an account for you.
#42
@
9 years ago
Linking the latest video by imath here for posterity.
https://vimeo.com/138356216
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
This ticket was mentioned in Slack in #buddypress by mercime. View the logs.
9 years ago
#46
@
9 years ago
So!
6570.06.patch is applying latest dev-chats decisions :
- Do not use/introduce the BP Modal
- Include a filter to change the default Attachments uploads dir.
I will work on splitting the patch into smaller parts and will ran some other tests (with BuddyPress standalone themes, BP Default..) in the next couple of days. Then if no objections, i will commit this first pass the day before next dev-chat.
First pass, because, after testing the feature again on our test drive http://imath-buddypress.wpserveur.net, you may miss the header's buttons to set the avatar and the cover image :)
imho, we should at least use the link to edit the cover image inside the cover image if bp_is_my_profile()
, we could also bring back the buttons with simply a link to the change-avatar/cover-image screens
Then, we will surely need to adjust some twenties companion stylesheets...
So first: does anyone has any objection about me committing 6570.06.patch this coming tuesday ?
#47
@
9 years ago
- Keywords commit added
Go ahead and commit! I've only one small suggestion:
- The message "For better results, make sure to upload an image having a width >= %1$s px and a height >= %2$s px". Using >= to mean "more than" (etc) is jargon. I think it should just say something like "...an image that's larger than ?px wide, and ?px tall."
#48
@
9 years ago
Thanks a lot for your feedback and suggestion DJPaul, you're absolutely right. I will change the message the way you suggested.
#49
@
9 years ago
I did some tests with specific cases :
1- BP Default, or add_theme_support( 'buddypress' )
themes
It behaves like it should: meaning no support for the Cover image, so we're playing nice with these themes :)
FYI: here is an example of how to get cover image for users using BP Default and i guess themes using add_theme_support( 'buddypress' )
:
https://gist.github.com/imath/7e936507857db56fa8da
2- A standalone BuddyPress theme using the Legacy template pack (e.g.: swifter)
I had one trouble with this case. Swifter is using specific templates but is also using Theme compatibility. So the feature is on even if templates are not ready yet (Admin Bar & User nav / Group nav / Group creation steps < this last one is annoying) So i guess it's a good example of the problems some themes (Using custom templates and BP Legacy) will have to deal with.
For themes like swifter, i've edited the check on the theme compat id to use bp_get_theme_compat_id()
. So swifter should tell to BuddyPress it is the current theme compat id doing at least something like this:
add_filter( 'bp_get_theme_compat_id', function( $theme_id = '' ) { return 'swifter'; }, 10, 1 );
If so, the cover image feature won't load because 'legacy' !== bp_get_theme_compat_id()
.
For other custom themes using the Legacy templates pack with some custom templates. The most annoying case is when the theme is overriding src/bp-templates/bp-legacy/buddypress/groups/create.php
. In this case the last group creation step is "Cover Image" and the custom template is not containing the corresponding nonce field, so it's not possible to create a group :(
Depending on the users need, if they wish to have time before implementing the feature, they can
- deactivate the corresponding BuddyPress settings (easy)
- use
add_filter( 'bp_is_groups_cover_image_active', '__return_false' )
andadd_filter( 'bp_is_profile_cover_image_active', '__return_false' )
Of course, by simply updating (or removing!) the overrided templates everything will work fine :)
This last case is the illustration of what @jjj was saying a month ago: https://wordpress.slack.com/archives/buddypress/p1439410307001361
"and no one else gets broken anything, other than maybe some themes that are already using theme compatibility out of the box"
I thought it was important that we were all aware of this potential edge case ;)
So I've updated the patch to latest trunk and applied @DJPaul's suggestion about the message. See 07.patch (you can check it easily by using our test drive).
Now i'm going to split the patch into smaller parts and i will soon begin to commit them ;)
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
9 years ago
#52
@
9 years ago
Sorry for the missing function names in the previous commit.
first bullet is bp_attachments_uploads_dir_get()
and it's like /wp-content/uploads/buddypress/members/1/cover-image
second bullet are bp_attachments_get_max_upload_file_size()
, bp_attachments_get_allowed_types()
, bp_attachments_get_allowed_mimes()
, bp_attachments_check_filetype()
third bullet is bp_attachments_get_attachment()
and bp_attachments_delete_file()
#61
@
9 years ago
Just made a build of trunk (r10159) and updated our test drive (http://imath-buddypress.wpserveur.net) to it. I've tesed it and everything works fine :)
#62
@
9 years ago
This looks great.
As the BP_Attachment class is used more and more, is there a plan to add a check for orientation of images?
As discussed in #5089 ?
Perhaps that issue already has its own ticket?
#63
@
9 years ago
I see @r-a-y posted a patch about the ticket you're mentioning, we just need to also apply it for the BP_Attachment_Cover_Image Class i guess.
Thanks for your feedback @shanebp, i will check #5089 and provide some feedback to r-a-y so that we can progress on it.
#70
@
9 years ago
Hi Guys,
Many thanks for such a great feature. I just have one doubt, why use CSS background-image
property to show cover photo and not img
tag?
#71
@
9 years ago
hi @rittesh.patel
i was about to close this ticket as i've just posted the codex page : https://codex.buddypress.org/themes/buddypress-cover-images/
i have no doubts! i'll try to convince you with a very intersting property for backgrounds:
background-size: cover
this property allowes us to scale the background image to be as large as possible so that the background area is completely covered by the background image.
it would be very difficult to arrive to the same result with the img tag.
if you still have a doubt we can carry on discussing, else let's close this ticket as fixed and use new tickets to report eventual issues.
+1 :)