Skip to:
Content

BuddyPress.org

Opened 2 years ago

Closed 21 months ago

#7551 closed enhancement (fixed)

Merge Nouveau template directory to BP core

Reported by: hnla Owned by:
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Templates Keywords:
Cc:

Description

This ticket manages the transfer of the github /bp-nouveau/ directory into BP core /bp-templates/

We'll handle fixes and further updates to files referencing this ticket.

After initial commit/patch we'll need to update BP's gruntfile so that Nouveau workflow can continue to compile scss, run watch tasks, rtl, copying from the github gruntfile, A quick re-factor locally stumbled on watch tasks which ran for gruntfile changes but none of the scss changes triggered; @netweb if you have a minute could you look at this?

Attachments (2)

nouveau-gruntfile.patch (877 bytes) - added by hnla 2 years ago.
Merge & update with watch task & lint paths
add-nouveau-to-theme-package-array.patch (794 bytes) - added by hnla 2 years ago.
Add Nouveau package details to the 'register_theme_packages()' function. Enables selection of package in BP settings.

Download all attachments as: .zip

Change History (26)

#1 @hnla
2 years ago

  • Type changed from defect (bug) to enhancement

#2 @r-a-y
2 years ago

A part of me believes that we shouldn't merge Nouveau into core yet. To me, Nouveau is still in beta.

Since we added the template switcher into core, we can keep Nouveau as a separate plugin so we can push fixes fast. Could even make the plugin update via Github exclusively if we use a Github update library.

If it is bundled, we cannot push fixes unless we release multiple maintenance releases and we have a tendency not to do frequent maintenance releases.

Just thought I'd raise these concerns.

#3 @hnla
2 years ago

@r-a-y hmm you make some very good points. working on github enables ease of quickly pushing fixes and that workflow will be lost a little, too your points about maintenance releases. Is Nouveau still Beta, it could be considered so yet we are at a beta release point?

Are you suggesting though that Nouveau remains as a plugin for ever more and if so that would likely devalue it's worth and effort expended to build it as I would expect people to not use it or be clear it exists?

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


2 years ago

#5 @r-a-y
2 years ago

I think this is an old problem that we had back in the bp-default days (as well as bp-legacy).

We can't push fixes unless we make a BP release. WordPress doesn't have this problem because their bundled themes can be updated independently from core releases.

The issues list in the Nouveau git repository makes me nervous. If we mark Nouveau as a beta, we can continue fixing things independently of BuddyPress. If we had something that is not considered a final release candidate product, support requests come in wondering why such-and-such isn't working or implemented. First impressions are important.

Last edited 2 years ago by r-a-y (previous) (diff)

#6 @hnla
2 years ago

Can't argue with those points, yes there's a fair few 'things' to deal with but lack of hands on help outside of core - remember we've/I've been battling away with this for over a year :(

My only overarching concern is this staying as a plugin as I think that will signal it's death knoll, but your points about first impressions are very valid, I envy WP being able to update ostensibly a bundled theme outside of a WP core release. if we could do that I would be happier.

#7 @netweb
2 years ago

Happy to help with whatever is required either way +1

#8 @hnla
2 years ago

@netweb Thanks, appreciate it

#9 @hnla
2 years ago

  • Milestone changed from 2.9 to 3.0

#10 @r-a-y
2 years ago

r11686 merged Nouveau into core (Thanks @hnla!).

Unit tests are failing due to the inclusion of minified assets. Will remove them in a subsequent commit.

#11 @r-a-y
2 years ago

In 11687:

Nouveau: After r11686, remove minified assets.

r11686 merged Nouveau into core, but unit tests are failing due to the
inclusion of minified assets.

See #7551.

#12 @r-a-y
2 years ago

Now that Nouveau's in core, I still believe that Nouveau should be a plugin. Read comment:2 and comment:5 for reasons as to why.

We would use a two-pronged approach similar to WordPress. WordPress bundles a bunch of default themes, but is still able to update them independently. How about we bundle Nouveau in core, but also keep it separate as a plugin so we can push updates fast.

The difference here is in BP core, we'd check to see if /wp-content/plugins/bp-nouveau/ exists before registering Nouveau internally as a template pack. If it exists, then we use that version of Nouveau. If it doesn't, we fall back to the Nouveau that is bundled with core.

The negatives with this approach are:

  • The duplication of code for Nouveau. There would be a /wp-content/plugins/buddypress/bp-templates/bp-nouveau/ directory and a /wp-content/plugins/bp-nouveau/ directory in this scenario.
  • For those not using the plugin version of Nouveau, they might have a slightly older version of Nouveau from the BP release version.

#13 @hnla
2 years ago

@r-a-y Thanks as I mentioned in Slack realised minified assets shouldn't have been uploaded, my bad, different dev environments, think this was partly why I didn't originally bother to compile minified assets in Nouveau - just logged on to delete them :) thanks for dealing with them..

Reading and contemplating your general comments.

Oh and thanks for linking the commit to ticket, I had forgotten this ticket existed :(

Last edited 2 years ago by hnla (previous) (diff)

#14 @hnla
2 years ago

Question: how does WP bundle core themes and yet allow for independent updates from core files?

#15 @hnla
2 years ago

One concern in respect to comment comment:2 - which I accept as a valid concern and approach - is that as a separate or somewhat disconnected 'thing' would it get any attention vis a vi updates, ongoing development or be conveniently forgotten about by core as something it doesn't have time to support?

Last edited 2 years ago by hnla (previous) (diff)

#16 follow-up: @r-a-y
2 years ago

Question: how does WP bundle core themes and yet allow for independent updates from core files?

The WordPress release ZIP file has a /wp-content/themes/ directory containing bundled themes.

Themes can be independently updated because of the wordpress.org themes repository and the WordPress core theme update system that writes to /wp-content/themes.

Obviously, BuddyPress doesn't have this luxury because WordPress only has a plugin and theme update system in core.

What I'm proposing is one way around this.

If we do not like the duplicate Nouveau directories, I have an idea that could possibly move the files out of /wp-content/plugins/bp-nouveau/ to /wp-content/plugins/buddypress/bp-templates/bp-nouveau during bp-nouveau plugin installation, but that would require adding some code in BP core to support this. With this approach, we would still need to keep a /wp-content/plugins/bp-nouveau/index.php file so WordPress can check the wp.org plugin repository for updates. One potential problem with this approach is with wp-cli or environments that have locked down file permissions. I haven't thought everything through yet; these are just some unfinished thoughts.

is that as a separate or somewhat disconnected 'thing' would it get any attention vis a vi updates, ongoing development or be conveniently forgotten about by core as something it doesn't have time to support?

The plugin version of Nouveau would have faster updates. For every BuddyPress release (maintenance or major), we would merge the changes from the latest plugin version of Nouveau to BP core.

#17 @netweb
2 years ago

In 11690:

Build Tools: Update Grunt tasks for Nouveau.

This change is a follow up to [11686] and updates the Grunt stylelint and sass tasks to support the new Nouveau template pack.

See #7551.

@hnla
2 years ago

Merge & update with watch task & lint paths

#18 follow-up: @hnla
2 years ago

@netweb when I originally tested nouveau directory copied out to run under core bp-templates I re-built bp core gruntfile locally so my working environment would work or at least tested to see if could work. Merged your last change and pushed up patch with my watch task and lint sections, if it helps.

One thing that occurred to me though is in the task 'src' we include 'sass' even though we would have had to compile sass previously so maybe we don't need to include that just keep to the lint, jshint, rtl etc?

#19 in reply to: ↑ 16 @hnla
2 years ago

Replying to r-a-y:

What I'm proposing is one way around this.

If we do not like the duplicate Nouveau directories, I have an idea that could possibly move the files out of /wp-content/plugins/bp-nouveau/ to /wp-content/plugins/buddypress/bp-templates/bp-nouveau during bp-nouveau plugin installation, but that would require adding some code in BP core to support this. With this approach, we would still need to keep a /wp-content/plugins/bp-nouveau/index.php file so WordPress can check the wp.org plugin repository for updates. One potential problem with this approach is with wp-cli or environments that have locked down file permissions. I haven't thought everything through yet; these are just some unfinished thoughts.

Well duplicate sets of files does feel a bit clumsy, but equally the point that the plugin version can be updated outside of BP version cycles is a strong one.

Having BP check to see if plugin version activated and use it if is, again get the point it would work but do we like the approach, feels a little hacky, there's a more elegant solution but we just haven't access to it.

I think I'm liking the sound of your alternative solution moving files around, but not overly keen on the idea that users have to install a plugin. It really feels to me as though we are hovering around the notion of our own means of apping WP theme update capabilities, if we could check the github repo for updates and download them to bp-nouveau if the pack was activated or upload the pack not as a plugin but as a set of theme files from github after we had run a build to WP repo and have BP update the core files and perhaps then grab github files as a dependency during the build process... but this is going down a dark complicated route and I'm waffling :)

I haven't thought everything through yet

If you can expand on your idea, as this does feel like quite a critical area.

#20 in reply to: ↑ 18 @netweb
2 years ago

Replying to hnla:

@netweb when I originally tested nouveau directory copied out to run under core bp-templates I re-built bp core gruntfile locally so my working environment would work or at least tested to see if could work. Merged your last change and pushed up patch with my watch task and lint sections, if it helps.

Thanks for the patch, I'll update and merge it into the patch for #6382 so that we can watch _all the things_

One thing that occurred to me though is in the task 'src' we include 'sass' even though we would have had to compile sass previously so maybe we don't need to include that just keep to the lint, jshint, rtl etc?

I'll take a closer look at that also

We're also currently "shipping" the SCSS files in the build/ folder which I'll also fix

#21 @hnla
2 years ago

Just a footnote to my earlier comment about re-running the sass task within the src one, it occurred to me it's actually quite convenient to be able to run everything from one command sometimes and seemingly has little consequence being re-run.

@hnla
2 years ago

Add Nouveau package details to the 'register_theme_packages()' function. Enables selection of package in BP settings.

#22 @hnla
2 years ago

In 11717:

Register Nouveau package in core

Commit adds the Nouveau package array parts to 'bp_register_theme_packages()`.

Allows activation of the Nouveau theme files from the BP settings option screen.

See #7551

This ticket was mentioned in Slack in #buddypress by codeart. View the logs.


2 years ago

#24 @DJPaul
21 months ago

  • Resolution set to fixed
  • Status changed from new to closed

This has been merged. I don't think a ticket this broad needs to stay open as we finish up Nouveau. I'd rather we open up specific tickets for any bugs or improvements we deem necessary for 3.0, so I'm closing this. :)

Note: See TracTickets for help on using tickets.