Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

Planet-2561 - Added customizations for campaign post types #445

Closed
wants to merge 5 commits into from

Conversation

mehulkaklotar
Copy link

@onyekaa onyekaa changed the title planet-2561 - campaigns cpt allowed in plugin blocks allowed types Planet-2561 - Added customizations for campaign post types Jan 30, 2019
@onyekaa
Copy link

onyekaa commented Jan 30, 2019

I've merged develop into this branch due to the changes made that cause conflicts with the work done so far. All theme styles and images from outlandish have been added in this PR.

Copy link
Member

@comzeradd comzeradd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @onyekaa!

Can you squash the commits and force push? So the merge commit disappears.

@@ -2,6 +2,7 @@
@import "base/variables";
@import "base/functions";
@import "base/mixins";
@import "base/blocks";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rather huge file. This should probably needs to break down into smaller components.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. How do you suggest I split them? By block types?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but as far as I can see this includes blocks that already exist. So the code should be split into the right components.

You use mixins to redefine some existing blocks, creating a lot of code repetition. It's not clear to me why this approach was chosen to suggest an alternative. But this will make the blocks prone to regression bugs if we need to do any change in the future. Maybe define the mixins right in their existing component, with some default values that you can override when you include them for the campaigns.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write this, this is the code gotten from Outlandish, so maybe we can discuss this with them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey guys, re the approach taken for the campaign styles ... my understanding is that we were provided with flat html and css for a showcase mock-up page and were asked to write css to apply the new designs to this html without altering the html or base css. This was before we were involved with working on the planet4 blocks plugin itself. Hope that's helpful?

@@ -187,3 +187,97 @@ $nested-comment-left-margin: $n50;
}
}
}

@mixin monochrome {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid adding a new set of media queries mixins. You can use the ones already in place and adjust your code to be mobile first.

assets/scss/themes/_theme_antarctic.scss Outdated Show resolved Hide resolved
assets/scss/themes/_theme_antarctic.scss Outdated Show resolved Hide resolved
assets/scss/themes/_theme_antarctic.scss Outdated Show resolved Hide resolved
@mcshanea
Copy link

@comzeradd squashing commits is usually quite a painful exercise, it moves us backwards in testing, and we can't invest continuous development time. Could you either jump in and do this yourself or we commit as is.

Reverted slug.

Style fixes for cover blocks, removed imports.

Moved font imports and recompiled css
@comzeradd
Copy link
Member

@mcshanea this is a standard review process for opening and merging PRs. Squashing would have been avoided if the feature branch was initially rebased, instead of merging. We can't really merge a PR with merge commits from the target branch.

See the git best practices that we tend to follow on planet4 (especially the "Amend" and "Rebase" parts).
https://planet4.greenpeace.org/handbook/dev-git-best-practices/

@onyekaa
Copy link

onyekaa commented Mar 26, 2019

@comzeradd Commits have been squashed.

@comzeradd
Copy link
Member

Closing this in favor of #526

@comzeradd comzeradd closed this Apr 5, 2019
@comzeradd comzeradd deleted the planet-2561 branch April 8, 2019 11:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants