buddypress / next-template-packs

is this the next BuddyPress template pack?
35 stars 9 forks source link

Locating ScSS partial files for default builds #108

Closed hnla closed 7 years ago

hnla commented 7 years ago

Agree whether this approach to keeping partial files located in /src/ but not compiled to builds is viable moving forward.

Currently I build the primary template pack scss file from a series of partial modular scss files these are intended to act as re-usable base styles for generic BP formatting on any new template pack. Once compiled to the primary template pack scss/css dir (during a development phase ) these files are no longer required moving forward and do not need to be part of a distributed BP package just available from trunk.

The principle here needs to fully tested and agreed/signed off on.

boonebgorges commented 7 years ago

Currently I build the primary template pack scss file from a series of partial modular scss files

How? I don't see a Gruntfile that handles this.

To clarify: you're proposing shipping the CSS file (which will of course be compiled into a single file) as well as a single SCSS file? That is, the option is between shipping only a single SCSS file that concatenates all your modular files, or shipping both the concatenated file and the modular ones?

hnla commented 7 years ago

How? I don't see a Gruntfile that handles this.

Currently the grunt files are local to ones dev environment, unlike BP dev where we provide a gruntfile it was less clear what exactly we needed, so didn't want to cross this bridge in terms of a common gruntfile until necessary.

However the scss compiling flow from a grunt point of view isn't much different we compile the src scss dir .scss to the css directory; however my watch task (currently BP doesn't but should implement watch task my local dev sites inc BP ones all modify gruntfile to include a watch task) is looking for changes in these dirs:

'bp-templates/bp-nouveau/sass/buddypress.scss',
'bp-templates/shared/styles/*.scss',
'bp-templates/bp-nouveau/sass/*.scss',
'Gruntfile.js',

The magic is actually in the partial import where scss handles this itself The main buddypress.scss file runs through the partial modules in this fashion:

// Import our generic navigation styles
@import "../../shared/styles/_bp_navigation";

and does this within a structured sectioned file. working in the partial files & saving forces the buddypress.scss file to compile to .css running all those @import statements.

Running up a new template pack would be a first pass process of copying this buddypress.scss to your new template pack in a similar structured dev environment running from BP trunk /src/ this way you immediately have a base set of styles imported, conversely ignore those @imports create your own file build styles from ground up.

I know that there are some issues with this approach - or indeed with any approach pros/cons :( - but for the moment it's a practical working approach.

hnla commented 7 years ago

The issue now is that we probably ought to have a gruntfile uploaded to facilitate others being able to setup tools, run tasks like compiling.

My local file is hacky and ugly it also implements post-css for linting as per netwebs desire to remove scss-lint in favour of post-css.

Perhaps it's time to upload the gruntfile and package.json.

boonebgorges commented 7 years ago

I'm afraid I get a bit lost in the conversation above. It sounds like there are a few issues:

  1. What should be included in BP trunk?
  2. What should be included in the BP distribution?
  3. What should be the workflow for a developer who wants to spin up his own template pack, using your modular SCSS files as a starting point?
  4. What Grunt tasks or other build processes should be provided by BP (trunk - distribution has no Gruntfile) in support of the above?

My take is that this repo is probably going to become defunct after we merge into BP. So, whatever files you think are going to be necessary for the long-term maintenance of bp-nouveau should live in BP trunk. If you think that holding onto the modular SCSS will be good for long-term maintenance, then we'll need to include all of the files in BP. This will mean that developers will need to use a watch task or browser tools when contributing to BuddyPress. You are likely to get pushback on this from some members of the team.

This is more a side-effect of using SCSS - I think the question of having many SCSS files vs one is a red herring. I don't personally see the benefit of combining all SCSS into a single file. The main obstacle to contributions is going to be SCSS watch tasks, not having lots of different files with style definitons.

I would approach this question not from the point of view of the tools or whatever, but in terms of balancing between (a) maintainability (ie having a modern setup that makes it easier to fix bugs and make improvements) vs (b) ease of contribution. Based on that, we can figure out what kinds of build tools we need.

hnla commented 7 years ago

Yep sorry it's one of those conversations that is always a little confusing, I'm not utterly sure of the answers here myself.

I'm nervous of the notion I perceive where we need to sort of have tools within tools and the confusion that might bring, but suppose that in reality once in core the main tools we already have serve both core and our new template directory and that we just need to adjust the scss compiling rules and add a watch task along the lines I'm currently using then we don't require tools actually in the /bp-nouveau/ directory at all ( I may be stating the obvious here)

Yes the github repo does become defunct, yet it's a current workflow that...works so would want to try and ensure that the move into core is handled as late as possible so we aren't having to adapt to a new work flow with any major work outstanding ( this may be my paranoia though).

To try and recap what I see the position as:

BP trunk gruntfile is modified to include new paths for Nouveau and a watch tasks.

The partial files live in trunk these are files that technically have served their purpose once you update one forcing the re-compiling of /bp-nouveau/scss/buddypress.scss/ to create our '/bp-nouveau/css/buddypress.css

At that stage assume we have our completed set of theme files along with our stylesheet(s).

Any changes or on the initial include we do as we would any other tim,e a commit to trunk, thereafter our process would be as we would do for a build where bp-legacy is included, now however building bp-nouveau .. but excluding a directory that now lives in /bp-templates/shared/ that isn't part of build only belonging to /src/ ( 'shared' is simply my working name it can be anything )

This will mean that developers will need to use a watch task or browser tools when contributing to

BuddyPress. You are likely to get pushback on this from some members of the team.

I get this but this then is a blocker to using Sass period, and do we chuck out a pre-procesor or tool because a few don't like it or are unfamiliar, it's less a rhetorical question more one of concern Technically opting to use a mechanism such as a pre-processor means that's the way all have to work once a project any project uses Sass/less you can no longer directly access or modify straight css files I'm now trying to think of better ways we could run things on this score.

In terms though of watch tasks / tools BP already makes it so that we have to use tools so the nature of this as an impediment already exists?

In terms of patching though people can contribute patches but patches where styles are concerned become the issue - I see no way around this though short of dumping Sass altogether or I have to manage the styles process completely - ease of contribution is going to be a big problem and issue :(

I don't personally see the benefit of combining all SCSS into a single file

The primary reason is keeping files small and easy to work in and modify, but we compile to one as we don't want multiple actual loaded stylesheets

boonebgorges commented 7 years ago

The primary reason is keeping files small and easy to work in and modify, but we compile to one as we don't want multiple actual loaded stylesheets

I mean I don't see the benefit of combining into a single SCSS file for trunk, which I thought you were originally proposing here. Obviously we should compile into a single CSS file for distribution.

In terms of patching though people can contribute patches but patches where styles are concerned become the issue

Will we commit compiled CSS files to trunk in addition to SCSS? I suppose this is what we currently do with companion stylesheets, yes? Doing this ensures that a raw checkout of BP trunk will result in a styled interface, and might be a good compromise to the purer position of "no compiled assets under version control".

The partial files live in trunk these are files that technically have served their purpose once you update one forcing the re-compiling

By this I think you mean that the files are untouched when you render a page. If so, then yes, the plan sounds right. But they haven't "served their purpose" insofar as we'll continue to use them as the source of truth when maintaining bp-nouveau styles going forward.

but excluding a directory that now lives in /bp-templates/shared/ that isn't part of build only belonging to /src/

I don't understand this part of the setup. Are the items in /shared/ copied to bp-nouveau (kinda like the _s theme is a "starter" theme) or are they dynamically loaded from /shared/? If they're copied, and we're only including them in trunk to serve as a starting point for future template packs, I think we should consider excluding them from the BP trunk repo. It's just more maintenance overhead that provides no short-term benefit (realistically, how many people will be creating template packs?).

hnla commented 7 years ago

I mean I don't see the benefit of combining into a single SCSS file for trunk, which I thought you were originally proposing here. Obviously we should compile into a single CSS file for distribution.

Ah ok this is more an artefact of the workflow at present - we compile at the point we make a change in a partial file, my requirement is that when making that change in a partial scss file in /shared/' I need my buddypress.scss to compile to the .css file this ensures I see the changes as I refresh browser view I need this to happen automagically. With the current workflow we are not technically working in trunk but in a plugin and as such any updates / changes need to be the full generation that allows the plugin to be downloaded, activated and for user to see all styles i.e as enqueued .css files.

Trunk thus would be pre-compiled and ready to go - as is BP currently i.e if we run BP from trunk /src/ we see a fully functioning styled set of bp screens.

Will we commit compiled CSS files to trunk in addition to SCSS? I suppose this is what we currently do with companion stylesheets, yes?

Yes! Trunk ( /src/) would have the .css file committed and I guess the rtlcss I see some potential issues (possibly, possibly not) unlike the companion styles which are additional sheets our primary style file buddypress.* only exists as editable/readable files in it's .css form the .scss form as a series of imports only acts as a skeleton framework for structure ( calling the imports at specific points in the file) thus it has served it's purpose before any commit to trunk, but still needs to be part of trunk - still this file hasn't existed before so I may be seeing problems where they don't exist.

So to answer the question fully - the raw checkout will be a fully working styled version and was always the intention and forced by the current watch tasks compilation flow, uncompiled assets in trunk simply doesn't work.

But they haven't "served their purpose" insofar as we'll continue to use them as the source of truth when maintaining bp-nouveau styles going forward.

Yes the /shared/ partial scss files need to be present in trunk /src/ otherwise we couldn't update styles without the various @imports failing on compilation run when working in trunk /src/ - however we don't include '/shared/ in the build process they do not want to be part of the distribution.

As to the last query - including these partials in trunk means we can update and would need them present, providing them for third party t/pack development is a courtesy but one where we're trying to establish a base set of 'good' styles for people to use, I do though agree this aspect of providing to developers isn't a massively important one but is it not cheap to include, does it bother us that trunk retains these files, is it a maintenance overhead, what does it mean we have to maintain? I'm open to views on this.

boonebgorges commented 7 years ago

Yes the /shared/ partial scss files need to be present in trunk /src/ otherwise we couldn't update styles without the various @imports failing on compilation run when working in trunk /src/ - however we don't include '/shared/ in the build process they do not want to be part of the distribution.

If they're necessary for building the package, then they need to be in trunk.

It's only unnecessary maintenance overhead if /shared/ exists alongside, and duplicates, some content in bp-nouveau (since we'd have to make fixes in two places). But if that's not the case, then it's not unnecessary overhead. What would the alternative be?

hnla commented 7 years ago

If they're necessary for building the package, then they need to be in trunk

In strict terms of building & package i.e running the grunt task 'build' then no they are not required for the build task to perform it's operations, however... Nor is this different than the workflow for our bp-legacy styles or companion styles.

If we need to effect a change in bp-legacy main styles we do so and do a straight commit back to trunk & to the /src/ folder, likewise companion styles are updated and I run $grunt commit to lint. & generate css / rtlcss files committing back to trunk. The aim being in these examples to ensure trunk templates are working examples when one checks out trunk and runs locally.

Where I am diverging from existing processes is in what we have in the respective assets of the theme folders /bp-legacy/css/ | bp-legacy/scss/ & /bp-nouveau/css/ | /bp-nouveau/scss/

Both themes hold the expected compiled or straight .css files, legacy also contains .scss files & their compiled counterparts and these files are all packaged in the build process.

Nouveau has an additional layer in it's scss flow budddypress.scss is the one and only stylefile (equivalent to legacy's buddypress.css). Unlike the companion scss files Nouveau's does not contain readable rulesets just the @imports required to pull in the partial files and compile to .css in one contiguous single operation on file change in turn committed to trunk (committing to trunk being the same process we follow currently)

My single concern in this flow is that we end up distributing a .sccs file (we distribute them in bp-legacy) that is only useful if one checks out trunk to have access to the partial files in /shared/ as you can't necessarily do any work in nouveau's buddypress.scss outside the trunk...but why would one?

and duplicates, some content in bp-nouveau (since we'd have to make fixes in two places)

No we don't duplicate anything the partials are the only actual css rulesets if you needed to change a property it's done in the partial automagically prompting the compile process to our css files; so we never need to fix in two places.

What would the alternative be?

Given the scss flow then I allowed for a fallback or optional change along these lines: I copy/paste the contents of a partial file e.g _bp-forms.scss, locate the @import section in buddypress.scss remove & replace with the actual rulesets. Now the partials are redundant & all rulesets live in our main scss file but the compile process remain unchanged as such my watch tasks don't change however now I'm making my changes in buddypress.scss and triggering the compile to .css as before.

I'm prepared to do this but have struggled to see a gain, the downside in my view is now I've locked styles into Nouveau and made it harder to re-use these style partials/modules for possible future themes/packs ( my overarching principle is always to explore ways of following DRY principles, BP styles are really not a trivial matter to build from the ground up )

On a slight tangent & at risk of further confusing but to highlight why I consider the partials located and used in my current approach was a way forward is the fact that I also allow for partial files in the actual template pack /bp-nouveau/scss/. Alongside the primary buddypress.scss file there are a few pack specific ones, these rules I consider are particular to something Nouveau has effected in component behaviour and which may not exist in another pack implementation:

/*
*-----------------------------------------
* @subsection 5.2.1.3 - Group Invites List
*-----------------------------------------
*/

// Import Group Invites screens

//@import "../../shared/styles/_bp_groups_invites";

// Import Nouveau style Group Invites screens

@import "_nouveau_invites";

Here we see that I've commented out the default styles in favour of Nouveau specific ones due to Nouveau having a unique layout requiring specific styles, the default styles are available but in this instance not required so we don't compile them. A future pack would be re-using this file but have removed the Nouveau import and re-instated the default global partial.

hnla commented 7 years ago

As per discussion I've moved partial scss files from shared/styles/ into bp-nouveau/common-styles/ & updated paths for compilation of buddypress.scss.