Skip to content

Conversation

@tomer
Copy link

@tomer tomer commented Dec 3, 2011

No description provided.

@debloper
Copy link
Owner

debloper commented Dec 4, 2011

  • Please provide little description with the pull-requests, about the introduced features.
  • Use the branch-root instead of ./MozillaCommunityTheme/
    • The entire repository is about Mozilla Community Sites' theme
  • I don't see the use of some image files committed in the code
    • Files such as background.png, footer-background.png are never used in the CSS
    • Please don't commit (yet) unnecessary files
    • If those files will be required in the next step, include them when they are required
  • Hell lotta files in one commit - feels like SVN strategy
    • It's tough for maintaining & tracing the code
    • Please commit step by step, implementing one feature at a time
  • The theme isn't standalone - tell me if I'm wrong about it
    • tomer@f9c427e#L9R13
    • We need standalone theme, not child-theme.
    • Even if it's a child theme (prefer not to) - re-implement it as standalone.

@tomer
Copy link
Author

tomer commented Dec 4, 2011

I've used subfolder as it easier to organize than dumping files at the
root. As a bonus, if at some point in the future there will be more themes,
it will be easier to maintain multiple themes at the same repository.
There are some files which are not used there. They are kept there
because they might be required soon and I've already created/retrieved
them.
This is a long commit because this is the first one in a series. I had to
copy over what we have already done.
As I said earlier, I do not plan to support a standalone theme as it is
completely useless. See the IRC log for the reasons.

On Sun, Dec 4, 2011 at 14:51, Soumya Deb <
reply@reply.github.com

wrote:

  • Use the branch-root instead of ./MozillaCommunityTheme/
    • The entire repository is about Mozilla Community Sites' theme
  • I don't see the use of some image files committed in the code
    • Files such as background.png, footer-background.png are never used in
      the CSS
    • Please don't commit (yet) unnecessary files
    • If those files will be required in the next step, include them when
      they are required
  • Hell lotta files in one commit - feels like SVN strategy
    • It's tough for maintaining & tracing the code
    • Please commit step by step, implementing one feature at a time
  • The theme isn't standalone - tell me if I'm wrong about it
    *
    tomer@f9c427e#L9R13
    • We need standalone theme, not child-theme.
    • Even if it's a child theme (prefer not to) - re-implement it as
      standalone.

Reply to this email directly or view it on GitHub:
#2 (comment)

Tomer Cohen
http://tomercohen.com

@debloper
Copy link
Owner

debloper commented Dec 4, 2011

  • There will not be more than one theme. Not primarily intended.
  • Even if there are more than one theme and you call this one "MozillaCommunityTheme" - what are you going to call the others?
  • If at that point of time, you give this theme a new name and/or change the directory name - it's technically similar to putting the files into a directory from root in Git's respect.
  • Include the extra files when they are included in the code
  • They may be required in future, but until then, it's not good practice to include them now.
  • Copy over files, that's okay - but don't stage/commit them until required.
  • As of making a child-theme, you're underestimating the MCS project.
  • We're making a theme - for people to make child themes on - if they need.

I suggest, keep it in you repository & let that be the official WP theme of MCS for now. Merging it here will not be required till we have better vision on the theme. If again a new "build-from-scratch" has to start, it'll be a mess in the repo.

Stabilize/complete this theme & let's release Israel & Antarctica site on your theme - we'll figure out by then.

@debloper
Copy link
Owner

debloper commented Dec 4, 2011

Nice work @amiad - the gradient also looks good-enough.
One thing though, follow @tomer change your tab-width to 4-spaces (it's 2 spaces now) - please maintain this consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants