-
-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Single project view #140
Single project view #140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a duplication of divs when loading a post via AJAX
</h2> | ||
|
||
<?php | ||
// we may need to redo these links as search query params instead | ||
$status = get_the_terms( get_the_ID(), 'project-status' ); | ||
$categories = get_the_terms( get_the_ID(), 'project-category' ); | ||
// @todo this throws errors when no terms are found for this type of post | ||
if ( is_array( $status) && is_array( $status ) ) { | ||
if ( is_array( $status) && is_array( $categories ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch. 👍
$(".projects-single-holder").html("Loading project..."); | ||
|
||
// actually load the single project | ||
$(".projects-single-holder").load(ajax_object.ajax_url+"?action=load_more_post&post_id="+post_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider https://current.test/ltw-test-page/ and the per-project page https://current.test/ltw-test-page/?project_id=3251741:
On page load, https://current.test/ltw-test-page/ displays in the single-project area the final post from the queue, which is the last item in the post list.
The DOM looks like this:
<div class="projects-single-holder">
<h1 class="projects-single-title">Youth Reporting Institute</h1>
....
</div>
Styles on .projects-single-holder
give the project a single black outline.
Clicking on the last item in the list loads the post:
<div class="projects-single-holder">
<div class="projects-single-holder">
<h1 class="projects-single-title">Youth Reporting Institute</h1>
....
</div>
</div>
The post now has a double black outline.
Loading the project-specific URL https://current.test/ltw-test-page/?project_id=3251741 means that that project is, upon page load, queried for by JS and then loaded by JS:
<div class="projects-single-holder">
<div class="projects-single-holder">
<h1 class="projects-single-title">Youth Reporting Institute</h1>
....
</div>
</div>
The post now has a double black outline.
In conclusion: loading the project via JS means that we get an extra div.projects-single-holder
.
- should we have separate divs for inner and outer, so that the AJAX response can reply with only the inner div to be appended to the outer? This might require splitting
partials/project-single-holder.php
into two files: the holder and the held.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benlk I noticed that too and wanted to hear your opinion on what you think would be best. I think splitting up the "holder" and the "held" sounds like a viable solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#140 (comment) explores a little more the holder/held distinction; I think what we'd end up with is:
div.project-single-layout
button.project-single-close
div.project-single-holder
article.project-single-held
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// set up basic args for our query to grab the post | ||
$single_project_args = array( | ||
'post_type' => 'projects', | ||
'p' => $post_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be sanitized; since post IDs are always integers we could do abs(intval( $post_id ))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 554eded
Not sure, but we should plan for attempted content overflow. Right now, iframes come in with
I don't think there's a separate item for this. At 770px and below, we could:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold/held looks good 👍
We can do the mobile styles and other work in a later pass, I guess?
Changes
This pull request makes the following changes:
project_id=ID
query paramWhy
For #125
Testing/Questions
Features that this PR affects:
Questions that need to be answered before merging:
Steps to test this PR: