Skip to content

Conversation

@brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Sep 23, 2022

Pull Request for Issue multiple .

Summary of Changes

Replace the very limited javascript used to display the diff between the core and the overriden file in com_templates with something much more useful

Testing Instructions

Apply this PR and then composer install and npm i
or use on of the prebuilt packages

There are two different views for the override: side by side and inline
side by side is the default - you can swtich to the inline version in the component options
both show the line numbers

A message is displayed if the override is identical to the core original file

Actual result BEFORE applying this Pull Request

too ugly to display ;)

Expected result AFTER applying this Pull Request

Default Side By Side View

image

Optional Inline View

image

Identical file View

image

Visible display of tabs and spaces

image

Documentation Changes Required

probably just updated screenshot

More Info

For more information on the library used and possible extras that could be added see https://github.com/jfcherng/php-diff

@richard67 a file is removed - shall I add it to the script or shall I ?

There may be some tweaks to the build scripts required to remove stuff from the library but I can only check when the package is built here ;)

@joomla-cms-bot joomla-cms-bot added Composer Dependency Changed Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Sep 23, 2022
@brianteeman brianteeman changed the base branch from 4.2-dev to 4.3-dev September 23, 2022 13:59
@brianteeman brianteeman requested a review from laoneo as a code owner September 23, 2022 13:59
@Kostelano
Copy link
Contributor

Cool feature!

A "double" border of 1 px on the block, because of this, the frame is thicker than on the blocks above. And an empty space in case we only need to change a couple of lines - it should be removed.

Screenshot_1

@Kostelano
Copy link
Contributor

Another suggestion: make the table header fixed when scrolling.

Screenshot_1

@brianteeman
Copy link
Contributor Author

@Kostelano I agree BUT I might have to ask for some help with the css from someone

@crystalenka
Copy link
Member

@Kostelano I agree BUT I might have to ask for some help with the css from someone

Add to that first row:

position: sticky;
top: 0;

@brianteeman
Copy link
Contributor Author

Not quite that simple.

You can only add it to a th not a tr. But that's not the problem I have. It's messing up all the borders

@crystalenka
Copy link
Member

If there is nothing in the old column (only difference is code added) the column widths go weird:

Screen Shot 2022-09-24 at 14 22 52

Adding the CSS:

tbody th {
	width: 5%;
}
tbody td {
	width: 45%;
}

Should fix it.

@crystalenka
Copy link
Member

Not quite that simple.

You can only add it to a th not a tr. But that's not the problem I have. It's messing up all the borders

Applying it to the thead th works. Regarding the borders-something to do with border-collapse on the table: https://stackoverflow.com/questions/50361698/border-style-do-not-work-with-sticky-position-element

@brianteeman
Copy link
Contributor Author

dont forget the css needs to work in both inline and sidebyside

@jfcherng
Copy link

jfcherng commented Sep 25, 2022

Those tables have different classes btw.

  • <table class="diff diff-html diff-inline">
  • <table class="diff diff-html diff-side-by-side">

@brianteeman
Copy link
Contributor Author

Thanks Jack - I will probably spend a little time later to create unique css for this using your tip

@brianteeman
Copy link
Contributor Author

will be updated - hopefu;lly soon - with support for displaying tabs and spaces

heelc29 added a commit to heelc29/joomla-cms that referenced this pull request Nov 6, 2022
[4.3] Template Diff View joomla#38823
richard67 added a commit to richard67/joomla-cms that referenced this pull request Dec 16, 2022
richard67 added a commit to richard67/joomla-cms that referenced this pull request Dec 16, 2022
richard67 added a commit to richard67/joomla-cms that referenced this pull request Dec 16, 2022
obuisard pushed a commit that referenced this pull request Dec 17, 2022
@RickR2H
Copy link
Member

RickR2H commented Mar 9, 2023

@brianteeman Just Double checking. I create an override and added some text. For some reason the whole compare area is colored. At first the added line was darker green after adding a second line. the darker color is gone. I'm on windows.

screen shot 2023-03-09 at 17 16 55

Just saving the override without adding text will add the full color background.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38823.

@brianteeman
Copy link
Contributor Author

This is what I have for the same changes

image

@RickR2H
Copy link
Member

RickR2H commented Mar 9, 2023

I just checked out the 4.3 branch and did an install. I'm curious what went wrong in my installation... I'll check tomorrow.

@jfcherng
Copy link

jfcherng commented Mar 9, 2023

the line ending changed?

@RickR2H
Copy link
Member

RickR2H commented Mar 9, 2023

Brian is on Windows too. Line endings shouldn't be the problem I guess.

@jfcherng
Copy link

jfcherng commented Mar 9, 2023

Just saving the override without adding text will add the full color background.

not sure whether he was aware of this "saving" step.

@RickR2H
Copy link
Member

RickR2H commented Mar 10, 2023

Did some further investigation. When I edit the file in VScode the diff works. When I edit and save the override in Joomla the color problem occurs.

diff-colors

@brianteeman Hope you have a tip or a fix. IMHO this is the most useful new feature in J4.3.

@jfcherng
Copy link

jfcherng commented Mar 10, 2023

I have strong feeling that this is a line ending issue. If it is, @brianteeman can rtrim($line, '\r\n') before sending to the diff lib. Or maybe I can add a new option to ignore line ending in my diff lib.

@brianteeman
Copy link
Contributor Author

@jfcherng I understand what you are saying but its odd that I cant replicate this

@RickR2H
Copy link
Member

RickR2H commented Mar 10, 2023

@brianteeman if you create a new PR, please let me know so I do the tests!

@jfcherng
Copy link

Maybe ask @RickR2H to look up what's actually stored in the database?

@brianteeman
Copy link
Contributor Author

@RickR2H please open a new issue with this report. that will get more visibility. Don't have time to look at it today.

richard67 added a commit to richard67/joomla-cms that referenced this pull request Apr 2, 2023
Delete the js files and the asset once deleted with PR's joomla#38823 and joomla#39374 and then added back with PR joomla#39431 for b/c reasons.
richard67 added a commit to richard67/joomla-cms that referenced this pull request Apr 2, 2023
Delete the js files and the asset once deleted with PR's joomla#38823 and joomla#39374 and then added back with PR joomla#39431 for b/c reasons.
richard67 added a commit to richard67/joomla-cms that referenced this pull request May 5, 2023
obuisard pushed a commit that referenced this pull request May 7, 2023
HLeithner pushed a commit that referenced this pull request Jun 26, 2023
…#39374 (#40302)

* Finally delete deprecated javascript assets

Delete the js files and the asset once deleted with PR's #38823 and #39374 and then added back with PR #39431 for b/c reasons.

* Add the deleted files to script.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Composer Dependency Changed Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester UI/UX

Projects

No open projects
Status: Review in Progress

Development

Successfully merging this pull request may close these issues.