Skip to content

Comments

[4.0] blog.php Scrabble96 patch 4#26160

Closed
Scrabble96 wants to merge 14 commits intojoomla:4.0-devfrom
Scrabble96:Scrabble96-patch-4
Closed

[4.0] blog.php Scrabble96 patch 4#26160
Scrabble96 wants to merge 14 commits intojoomla:4.0-devfrom
Scrabble96:Scrabble96-patch-4

Conversation

@Scrabble96
Copy link
Contributor

Pull Request for Issue #26123 .

Summary of Changes

Removed two lines of code (79 and 84) as IE11-related and not necessary.

Testing Instructions

Create com_content category override. Remove the two lines from blog.php
Create or view a category blog.

Expected result

The blog should display correctly

Actual result

Documentation Changes Required

Scrabble96 and others added 8 commits January 12, 2019 17:11
The existing instructions are for Joomla 3x
4.0 dev correct PR 201908151704
4.0 dev updated 201908171108
4.0 dev Merge from origin 2019-09-01
Remove two lines of code (lines 79 and 84) as IE11-related and irrelevant
@Scrabble96
Copy link
Contributor Author

Scrabble96 commented Sep 4, 2019

Ok. I'm a bit confused (as usual).

@Quy The original lines 79 to 84 were like this:
<div class="blog-item-content"><!-- Double divs required for IE11 grid fallback -->
<?php
$this->item = & $item;
echo $this->loadTemplate('item');
?>
</div>
(can't get the indentations to show here)

I removed the first and last line, so it should have looked like this for lines 79 to 82 inclusive:

<?php
$this->item = & $item;
echo $this->loadTemplate('item');
?>

I don't know why the opening and closing php tags are not lined up. They were when I edited the file in my repository.

@zero-24 You are asking me to change the single php code with two items into two separate php items like this:

<?php $this->item = & $item; ?>
<?php echo $this->loadTemplate('item'); ?>

PHP is definitely not my field of expertise, so could you explain why they need to be two separate instructions? Thanks.

@Scrabble96
Copy link
Contributor Author

And what does 'Resolve conversation' mean?

Re-aligned php tag in line 79
@Scrabble96
Copy link
Contributor Author

Sorry (again). I'm looking at Github's instructions (https://help.github.com/en/articles/reverting-a-pull-request) but do not see 'Revert' anywhere:

It might be easiest just to close this one and start all over again.

@N6REJ
Copy link
Contributor

N6REJ commented Sep 4, 2019

image
is this right?

@brianteeman
Copy link
Contributor

@N6REJ that is nothing to do with this PR. Please create your own issue instead of introducing new issues to an existing pr

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 5, 2019

There's some CSS associated with blog-item-content class

It will need to be updated.

@Scrabble96
Copy link
Contributor Author

There's some CSS associated with blog-item-content class

It will need to be updated.

In that case, why not just leave the file as it was and only remove the IE11 comment from the file instead of the whole div?

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 5, 2019

If those nested divs were added only because IE needed them, it would be nice to have them removed. SCSS changes are pretty simple. Just need to move .blog-item-content rules to .blog-item.

@Scrabble96
Copy link
Contributor Author

Scrabble96 commented Sep 6, 2019

If those nested divs were added only because IE needed them, it would be nice to have them removed. SCSS changes are pretty simple. Just need to move .blog-item-content rules to .blog-item.

Ignore - commented incorrectly, so removed it.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 6, 2019

Relevant SCSS code is linked in the comments above.

@Scrabble96
Copy link
Contributor Author

Relevant SCSS code is linked in the comments above.

Sorry, yes, that's why I removed my comment. I couldn't see how to delete it.

@Scrabble96
Copy link
Contributor Author

Scrabble96 commented Sep 6, 2019

Ok. I've looked at the scss files, but as I have zero experience using scss I can't offer to change this code. There's some code for @media (max-width: 767.98px) that appears in the template.css but not in the above scss file, so that must be held somewhere else. If I attempt to change this code I'll probably screw it up.

@N6REJ
Copy link
Contributor

N6REJ commented Sep 8, 2019

@N6REJ that is nothing to do with this PR. Please create your own issue instead of introducing new issues to an existing pr

@brianteeman
Like usual you have to be a jerk and ignore the item... This is part of the file she's submitting so why would this NOT be the proper time to bring it up. STOP BEING A JERK!

@N6REJ
Copy link
Contributor

N6REJ commented Sep 8, 2019

@Scrabble96 @SharkyKZ I believe what he's saying is you could move 157-197

  flex-direction: column;
  .boxed & {
    background-color: #fff;
    box-shadow: 0 0 2px rgba(52, 58, 67, 0.1), 0 2px 5px rgba(52, 58, 67, 0.08), 0 5px 15px rgba(52, 58, 67, 0.08), inset 0 3px 0 $cassiopeia-template-color;
    .item-content {
      padding: 25px;
    }
  }
  .image-left &, .image-right & {
    flex-direction: row;
  }
  .item-image {
    margin-top: 3px;
    margin-bottom: 15px;
    overflow: hidden;
    .boxed & {
      margin-bottom: 0;
    }
    .image-right & {
      order: 1;
    }
    .image-bottom & {
      order: 1;
      margin-top: -15px;
    }
  }
  .item-content {
    .image-left & {
      padding-left: 25px;
    }
    .image-right & {
      padding-right: 25px;
    }
  }
  .image-left &, .image-right & {
    flex-direction: row;
    @include media-breakpoint-down(sm) {
      flex-direction: column;
    }
  }

and move it between 93 & 94 so that it would be this..


 .blog-item {
  padding: 0 ($cassiopeia-grid-gutter / 2) $cassiopeia-grid-gutter ($cassiopeia-grid-gutter / 2);
    display: flex;
  flex-direction: column;
  .boxed & {
    background-color: #fff;
    box-shadow: 0 0 2px rgba(52, 58, 67, 0.1), 0 2px 5px rgba(52, 58, 67, 0.08), 0 5px 15px rgba(52, 58, 67, 0.08), inset 0 3px 0 $cassiopeia-template-color;
    .item-content {
      padding: 25px;
    }
  }
  .image-left &, .image-right & {
    flex-direction: row;
  }
  .item-image {
    margin-top: 3px;
    margin-bottom: 15px;
    overflow: hidden;
    .boxed & {
      margin-bottom: 0;
    }
    .image-right & {
      order: 1;
    }
    .image-bottom & {
      order: 1;
      margin-top: -15px;
    }
  }
  .item-content {
    .image-left & {
      padding-left: 25px;
    }
    .image-right & {
      padding-right: 25px;
    }
  }
  .image-left &, .image-right & {
    flex-direction: row;
    @include media-breakpoint-down(sm) {
      flex-direction: column;
    }
  }
}

you could then remove the empty 

.blog-item-content {
}

@mbabker
Copy link
Contributor

mbabker commented Sep 8, 2019

@N6REJ Adding unrelated changes to a pull request complicates matters. Just because someone is changing a file does not give carte blanche to request every line in that file be reviewed or changed. Patches should focus on a singular change only.

@N6REJ
Copy link
Contributor

N6REJ commented Sep 8, 2019

@N6REJ Adding unrelated changes to a pull request complicates matters. Just because someone is changing a file does not give carte blanche to request every line in that file be reviewed or changed. Patches should focus on a singular change only.

@mbabker ty. As you know I would never presume I have carte blanche, but your response explained things better. Thanks

@N6REJ N6REJ mentioned this pull request Sep 8, 2019
@N6REJ
Copy link
Contributor

N6REJ commented Sep 8, 2019

ugh, I always make a mess... @Scrabble96 hope that helps you.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 8, 2019

@mbabker This is not an unrelated change. SCSS must be updated to account for markup changes in this PR. Otherwise the styling is broken.

@Scrabble96
Copy link
Contributor Author

Thank you @N6REJ and @SharkyKZ
I'll have a look later today at adding that code. It makes sense as my PR fails otherwise.

@mbabker
Copy link
Contributor

mbabker commented Sep 8, 2019

Not the SCSS, the other question about the item var assignment.

@Scrabble96
Copy link
Contributor Author

I'm not sure what I'm supposed to be doing, now. @N6REJ has made the SCSS changes into his own branch, but it appears to have failed checks, so if I tried the same thing, wouldn't it have the same result?

This PR is showing as 'All checks have passed' What happens next?.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Sep 9, 2019

Failed checks unrelated. If SCSS changes are not included in this PR then #26208 should be merged first before this can be tested. In the meantime could you remove the second instance of the same code and fix indentation?

<div class="blog-item-content"><!-- Double divs required for IE11 grid fallback -->

@Scrabble96
Copy link
Contributor Author

In the meantime could you remove the second instance of the same code and fix indentation?

I've deleted a branch hope that's achieved what you want which is I think where the other instance of the changed file was. This repo thing is very, very confusing to a new user.
I've looked at the changed file on here and as far as I can see the paired div tags are lined up, so I don't know what else needs changing.

@brianteeman
Copy link
Contributor

@N6REJ No. Thats not what I said. You have to submit them as pr here https://github.com/Scrabble96/joomla-cms/tree/Scrabble96-patch-4

@N6REJ
Copy link
Contributor

N6REJ commented Sep 9, 2019

@brianteeman if that doesn't fix it I give up.. idk how to do anything else.

@ciar4n
Copy link
Contributor

ciar4n commented Sep 9, 2019

#26238 (comment)

If true then this would also need to be closed

@Scrabble96
Copy link
Contributor Author

@N6REJ and @brianteeman I have no idea what's going on now. PRs have been submitted to my patch. What do I do with them?

If I click 'Resolve conflicts' it just highlights some code but doesn't say what the conflict is. I suspect that my original change to the SCSS has been outdated by @ciar4n's changes for mobile-first code in PR #26237.

@Scrabble96
Copy link
Contributor Author

Ok. I'm going to close this. If someone else wants to start again they are welcome.

@Scrabble96 Scrabble96 closed this Sep 11, 2019
@Scrabble96 Scrabble96 deleted the Scrabble96-patch-4 branch September 18, 2020 16:14
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.

10 participants