Version 1.2 #4

twiro · 2014-08-14T15:44:03Z

This Update introduces two new features to this extension:

Anchor Label: Adding an optional custom label for the links on the publish pages
Display URL: Display URL in entries table (Instead of anchor label)

Behaviour:

Page Publish Entry:

If an "Anchor Label" is set it is used as the title of the preview link.
If no "Anchor Label" is set the preview link title will fall back to "Preview".

Page Entries Table:

If "Display URL" is checked the full URL will be shown as the title of the preview link.
If "Display URL" is not checked, but an "Anchor Label" is set this will be used as the title of the preview link.
If "Display URL" is not checked and no "Anchor Label" is set the preview link title will fall back to "Preview".

In addition to that I made the entry id available for url formatting and did some small changes & additions in the readme.

It's working fine for me and I tried to adapt as much of your coding-style as possible, but as I'm not an experienced php-coder it might be better if you could review these changes rather carefully ;)

Introduces two new features to this extensions: - Anchor Label: Adding an optional custom label for the links on the publish pages - Display URL: Display URL in entries table (Instead of anchor label) See #3

nitriques · 2014-08-15T00:45:49Z

@twiro This is amazing! Thanks!

One thing tho: How would you feel about renaming the param to {$system:id} since

That's the 'key' used in datasources.
'id' can (and will) be a field handle.
we already have some 'system:' prefixed values.

?

twiro · 2014-08-16T18:45:12Z

One thing tho: How would you feel about renaming the param to {$system:id} since

That's the 'key' used in datasources.

ok, that's a valid point ;)

'id' can (and will) be a field handle.

No more since Symphony 2.4 - 'id' as field label or field handle has been disallowed there:
symphonycms/symphonycms@c6f3688

we already have some 'system:' prefixed values.

I noticed them and was a little irritated about their supposed functionality - I first thought these would deliver some formatted date-values belonging to the entry, but noticed they were actually used for formatting the current date/time at the moment the function is called.

That's actually the reason I thought using the "system"-prefix for the entry ID wasn't the right way to go - it would mix things up that don't really belong together: Other than in the datasource-editor where the "system"-prefix is used for values that are generated by the system but actually do belong to the entry, the current implementation of the "system"-prefix in this extension just takes care of values that don't belong to the entry.

Because of that I simply routed for the easiness of {$id} :)

But I don't have a problem with {$system:id} if you think that thould be the cleaner option.
Would I have to somehow fix this PR or send a completely new one - never changed a PR before ;)

On another note, I just noticed that theses "system"-variables don't seem to actually work right now. Will send a separate issue for that.

nitriques · 2014-08-20T03:48:22Z

c6f3688

Did not knew about this thanks!

'system' in that case means "not from a field". Fields holds the "main" data of an entry, the rest is "meta" (wow lots of double quotes). That was use to prevent clash with actual fields name. As for the rest see issue #5.

Would I have to somehow fix this PR or send a completely new one - never changed a PR before ;)

Please update this one!

I did listen and read lots of git talks and I will gladly share my git-fu with you. Since you already started like a good git citizen (you are working on a feature branch) you are facing two valid options (2 and 3 are the same, 2 is just a special type of 3).

You just push new commits to your 1.2 branch. They will automatically be added to this PR, since a PR does not actually exists in git (it's a github thing). (Side note: git knows branches and a pr is a special branch. In fact, creating a pr creates a hidden branch in the git repo that lives on github's server. I could even pull it in, without touching your git repo on github's server by issuing git pull origin pull/4/head. Notice that origin here is the DeuxHuitHuit's github repo, not yours. End of side note) This is the lazy way out, but is accepted amongst almost every open source project.
If the PR contains only one commit: You amend your previous commit. This may seems scary (as you are going to re-write history) but is considered better by purist since you update the branch to contains only commits that are atomically valid and does not contains anything that does not to need to be reviewed. Also, this takes on the fact that git never forgets about anything: even after you re-write history, your old commit will still be there, but will not be referenced by the new tree you are going to create.

In order to amend your commit, you will start by actually changing the source file and make sure it works. Then, when you are ready to commit your changes, issue git commit -a --amend This will bring up vi with the previous commit message. Edit the message (if necessary) save and exit. BOOM. Your changes are now squashed (a git term) or melted with the previous one, now containing only the right thing.

If the PR contains multiple commits (as it does here), you are going to re-write more than one commit, which is even scarier. You first commit the things that needs to be changed as you would do with 1, but before you push, you rebase. The thing who want to tell git is "please rebase that branch to this commit minus one in interactive mode" which should be, in our case, git rebase -i 19e15b6^1. See the minus one (^1) ? Then vi will show up with a list of all your commits, including 19e15b6. Let the first line be the same, but edit all other and change 'pick' to 's' or 'squash'. Save and exit. Git will then merge all of the commits, re-writting history of your local branch and re-show vi in order for your to edit the commit message of this new giant commit. You could even choose which commit to pick and which to squash into the previous pick. At each merge, you'll have the chance to edit the commit message. This is, really butyfull stuff once you're confortable with it and you see how brilliant Linus Torvald is: With svn he had to deal with those merges, now it's the burden of the contributor.

If you picked 2 or 3, git push origin 1.2 WILL FAIL. It's normal, since you are overwritting the tree and creating orphan commit(s). Just add --force to tell git to go fuck itself and let you do it because you know what you are doing right?

BEWARE: Never rewrite history on the master/integration branch, only feature/patch branches. Anybody that had previously pulled in your 1.2 branch will get hangry at you because their git pull call failed. You did not re-wrote history for your self, but for everybody. But since your on a feature branch, that nobody had any reason to pull in (except for review) it's safe! I would also recommend to test those things out in another branch created from 1.2.

More on that: https://www.atlassian.com/en/git/tutorial/rewriting-git-history

I like to post things like that 😄 but it's late here... please forget the typos and ask questions if I am not clear.

nitriques · 2014-08-20T03:49:18Z

Sorry markdown let me down on this one. The second 1 should be 3.

To keep backwards-compatibility I didn't remove the old system-date-formatting. So there are two methods to access system-dates right now: **1) $system:variable** Used for accessing the entry-id and system-dates "the old way" - $system:time - $system:date - $system:year - $system:id - ... **2) $system:variable:qualifier** Used as "the new syntax" to access system-dates. Follows the qualifier-concept used for the field-data and allows using all qualifiers for php date_format - e.g: - $system:date:d - $system:date:Y - $system:date:H:i - ...

twiro · 2014-08-20T17:23:44Z

Wow. Thank you very much!
I went with option Nr. 1 as this one felt the safest for a complete git-noob like me, but I really appreciate all the insights you're sharing here... I hope I'll find the time to dig a little deeper into all this soon - right now I'm already happy if I somehow get to submit a valid PR ;)
Let me know what you think about this update and if you're fine with it I'll update the readme to match these changes.

nitriques · 2014-08-22T01:34:48Z

Great. I will test it ASAP and merge if everything went well. Thanks again!

twiro · 2014-08-27T15:19:39Z

Great. I will test it ASAP and merge if everything went well. Thanks again!

Don't forget that the Readme still needs to include my latest changes - if you're ok with them.
Just leave me a note after you checked my PR and I'll update it if you want.

By the way - what do think about renaming this extension to "Frontend Link"? Would reflect the changes in the 1.2-update and somehow communicate more flexibility than focussing on the preview-aspect.

"Dynamic Frontend Link" might be a good alternative too, as the dynamic-aspect might be the monst important difference to the "Entry URL"-extension.

Well - just an idea... I know one shouldn't rename extensions without good reasons ;)

nitriques · 2014-08-27T15:49:48Z

Don't forget that the Readme still needs to include my latest changes

Do you want to add them ? :smille: Simply add commit to this PR!

I know one shouldn't rename extensions without good reasons ;)

Yeah, I'll think about that, thanks for the suggestion. I think it should only the Dynamic Link since it's not mandatory to be Frontend related.

nitriques · 2014-08-28T01:50:54Z

@twiro I finally had the chance to review everything and it looks really sharp! Thanks!

I you feel to contributing again please pick up any of the remaining tasks:

Use a more Symphony-ish layout for the field's settings (using tow cols and col class)
Bring the "preview" header link that is currently broken (use data attribute to fix things up)
Allow the use of the {$param} syntax in the anchor label as well!

I will try to do my best to clear those ASAP. Thanks again.

Reflects the changes made to the syntax for formatting system-values as discussed here: #5 #4

inluding `:date:` without code-formatting shows a github emoji ;)

twiro · 2014-08-28T09:58:42Z

I finally had the chance to review everything and it looks really sharp! Thanks!

Thank you - great to hear it works for you :)

Do you want to add them ? :smille: Simply add commit to this PR!

Readme ist updated and reflects the latest changes I made.

[x] Use a more Symphony-ish layout for the field's settings (using tow cols and col class)

Included this change in this PR.

[ ] Bring the "preview" header link that is currently broken (use data attribute to fix things up)

What's wrong with it currently?

[ ] Allow the use of the {$param} syntax in the anchor label as well!

Yeah, that would be a nice addition too... do you want to include this in version 1.2 or put this in a later release? I might give this a try these days, but I'm not sure if I'm able to solve this in a clean way ;)

I think it should only the Dynamic Link since it's not mandatory to be Frontend related.

Valid argument... though the readme currently suggests exactly that ;)
https://github.com/DeuxHuitHuit/link_preview/blob/master/README.md#specs

What about "Dynamic Entry Link" or "Dynamic Link Field" - these would both promote the dynamic aspect without focussing on the frontend and communicate clearly that this extension is entry/field-related.

Including an example image in the readme might improve understanding even more:

nitriques · 2014-08-28T14:08:25Z

What's wrong with it currently?

I simply do not see it. Maybe it was because I tested it on 2.5rc1 ?

Yeah, that would be a nice addition too... do you want to include this in version 1.2 or put this in a later release?

I link it yes! I might have the time to do it this week-end since its a 3 days weekend here in Canada.

Valid argument... though the readme currently suggests exactly that ;)

Yeah. That's wrong!

Dynamic Link Field

I really like this. But let's release 1.2 and then renamed it for 2.0

twiro · 2014-08-28T14:39:36Z

I simply do not see it. Maybe it was because I tested it on 2.5rc1 ?

mmh... I tested both with 2.3.6 and 2.5RC1 and the header link is working as expected in both environments - did you do a fresh install or update the extension? I think I did not check the update process with 2.5RC1 - will check that ASAP.

I really like this. But let's release 1.2 and then renamed it for 2.0

Sounds good 👍

nitriques · 2014-08-28T14:47:22Z

did you do a fresh install or update the extension?

Fresh one.

twiro · 2014-08-29T09:06:05Z

I simply do not see it. Maybe it was because I tested it on 2.5rc1 ?

Does setting an "anchor label" make any difference?

I did not touch the javascript that actually generates the link for the header, so I guess this might be caused by an empty data-text-attribute set in displayPublishPanel.

But if no anchor-label is found it should simply fall back to the default term "Preview".
Is there anything wrong with this line I added to displayPublishPanel?

$label = $anchor_label != '' ? $anchor_label : __('Preview');

Should I change it to:

$label = !empty($anchor_label) ? $anchor_label : __('Preview');

But I guess that won't fix the problem... any ideas?
I just did another fresh install on 2.5RC1 and got no problems with the header link.

Make the two new inputs fields on two columns. Some code refactoring.

nitriques · 2014-10-21T20:37:04Z

Hey Roman, I finally had the time to try this out... Would you mind testing my pr-4 branch ? https://github.com/DeuxHuitHuit/link_preview/tree/pr-4

nitriques · 2014-10-22T15:06:34Z

Everything have been merged into the dev branch... https://github.com/DeuxHuitHuit/link_preview/tree/dev

twiro · 2014-12-05T14:18:41Z

Thanks for merging - looks & works great!

While testing this in a new project I noticed that I also need IDs of associated entries... so a new PR is already in its way :)

nitriques · 2014-12-05T20:56:23Z

While testing this in a new project I noticed that I also need IDs of associated entries... so a new PR is already in its way :)

😄

But for which field do you need it ?

twiro added 6 commits August 14, 2014 17:05

Update to 1.2

19e15b6

New Settings "Anchor Label" and "Display URL"

cf53daf

Introduces two new features to this extensions: - Anchor Label: Adding an optional custom label for the links on the publish pages - Display URL: Display URL in entries table (Instead of anchor label) See #3

Include entry ID by using the url format {$id}

1be51cd

Update extension.meta.xml to 1.2

b2caedd

Update README.md

2c26f1f

Fixed typo

e9f80f2

This was referenced Aug 14, 2014

Proposal: Configurable "Anchor Label" instead of hardcoded term "Preview" #3

Closed

Access entry ID #2

Closed

twiro added 3 commits August 28, 2014 11:16

Use a more Symphony-ish layout for the field's settings

a6e88e6

Update README.md

ec6fb68

Reflects the changes made to the syntax for formatting system-values as discussed here: #5 #4

Fixed formatting

6ed543b

inluding `:date:` without code-formatting shows a github emoji ;)

nitriques added this to the 1.2 milestone Aug 29, 2014

nitriques self-assigned this Aug 29, 2014

nitriques added the enhancement label Aug 29, 2014

nitriques added a commit that referenced this pull request Oct 21, 2014

Follow up of PR #4: UI adjustements

4eead08

Make the two new inputs fields on two columns. Some code refactoring.

nitriques closed this Oct 22, 2014

twiro mentioned this pull request Dec 5, 2014

Proposal: Rename extension to "Dynamic Link Field" #10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version 1.2 #4

Version 1.2 #4

twiro commented Aug 14, 2014

nitriques commented Aug 15, 2014

twiro commented Aug 16, 2014

nitriques commented Aug 20, 2014

nitriques commented Aug 20, 2014

twiro commented Aug 20, 2014

nitriques commented Aug 22, 2014

twiro commented Aug 27, 2014

nitriques commented Aug 27, 2014

nitriques commented Aug 28, 2014

twiro commented Aug 28, 2014

nitriques commented Aug 28, 2014

twiro commented Aug 28, 2014

nitriques commented Aug 28, 2014

twiro commented Aug 29, 2014

nitriques commented Oct 21, 2014

nitriques commented Oct 22, 2014

twiro commented Dec 5, 2014

nitriques commented Dec 5, 2014

Version 1.2 #4

Version 1.2 #4

Conversation

twiro commented Aug 14, 2014

Behaviour:

nitriques commented Aug 15, 2014

twiro commented Aug 16, 2014

nitriques commented Aug 20, 2014

nitriques commented Aug 20, 2014

twiro commented Aug 20, 2014

nitriques commented Aug 22, 2014

twiro commented Aug 27, 2014

nitriques commented Aug 27, 2014

nitriques commented Aug 28, 2014

twiro commented Aug 28, 2014

nitriques commented Aug 28, 2014

twiro commented Aug 28, 2014

nitriques commented Aug 28, 2014

twiro commented Aug 29, 2014

nitriques commented Oct 21, 2014

nitriques commented Oct 22, 2014

twiro commented Dec 5, 2014

nitriques commented Dec 5, 2014