Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

[WIP] Implement PSTT theme #38

Closed
wants to merge 26 commits into from
Closed

Conversation

DavidCramer
Copy link
Contributor

@DavidCramer DavidCramer commented Apr 11, 2018

Huge work in progress.

  • extract the logic in the PSTT functions.php file into the includes/functions.php file that’s now in develop.
  • Keep the require statements from the PSTT functions.php file.
  • Keep classes separate in includes/classes.
  • Bootstrap features in AMP_Travel_Features to init themes extractable features.

^ cc @kienstra

Fixes #3

@DavidCramer DavidCramer requested a review from kienstra April 17, 2018 08:53
@DavidCramer
Copy link
Contributor Author

@kienstra - could you take a look at this and see if it looks correct according the the task?

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Thanks, Some Points Below

Hi @DavidCramer,
Thanks for this huge effort. Most of these comments aren't regarding your work, only whether certain elements of the "WP Rig" theme will go along with our theme well.

This is an open discussion, so feel free to reply where you think something is needed.

comments.php Outdated
@@ -0,0 +1,78 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this, but we'll probably use the comments.php from PR #39. That has AMP-specific markup, like <amp-live-list>.

@@ -0,0 +1,10 @@
/*--------------------------------------------------------------
Copy link
Contributor

@kienstra kienstra Apr 17, 2018

Choose a reason for hiding this comment

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

This file probably won't be needed.

@@ -0,0 +1,176 @@
/*--------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably isn't needed for now, as I haven't seen widgets in the design.

footer.php Outdated
@@ -0,0 +1,41 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably use the footer.php from #39, as it's very specific to the design.

header.php Outdated
@@ -0,0 +1,52 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably use the header.php from #39, as it's also very specific to the designs.

The minimum version set here should be in line with the minimum WP version
as set in the "Requires at least" tag in the readme.txt file. -->
<rule ref="WordPress.WP.DeprecatedFunctions">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this rule not needed?

@@ -58,5 +83,42 @@

<exclude-pattern>*/node_modules/*</exclude-pattern>
<exclude-pattern>*/vendor/*</exclude-pattern>
<exclude-pattern>*/html/*</exclude-pattern>
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to add this.

-->


<config name="testVersion" value="5.2-99.0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like travis.yml checks for PHP 5.6, 7.0, 7.1, and 7.2. Should this be:

value="5.6-99.0"

@@ -0,0 +1,19 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like the design has a sidebar, so I don't think we need this file. Still, I'm not sure if there are plans for a sidebar later.

@@ -13,5 +13,893 @@ Tags: custom-background, custom-logo, custom-menu, featured-images, threaded-com
This theme, like WordPress, is licensed under the GPL.
Use it to make something cool, have fun, and share what you've learned with others.

This stylesheet is intentionally left blank.
CSS normalization based in part on normalize.css by
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably have to see how the front-end work goes in PRs like #39. But as our styling is specific to the design, we probably won't need a lot of this styling.

@mehigh mehigh closed this Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring PSTT theme code to this repo and document any findings
3 participants