Skip to content
This repository has been archived by the owner on Nov 22, 2021. It is now read-only.

Geen lege albumnaam weergeven als de human name niet is ingevuld #38

Closed
wants to merge 1 commit into from

Conversation

Minnozz
Copy link
Contributor

@Minnozz Minnozz commented Jan 13, 2013

In plaats daarvan de pathname gebruiken.

Niet getest, omdat ik geen test-setup heb.

$displayname is the album's $humanname, with a fallback to $name
@aykevl
Copy link
Contributor

aykevl commented Jan 13, 2013

In install.txt staat heel kort hoe knfotos geïnstalleerd moet worden.

@bwesterb
Copy link
Member

@Jille, @aykevl93 kan een van jullie kijken of dit ok is?

@ghost ghost assigned Jille Jan 14, 2013
@aykevl
Copy link
Contributor

aykevl commented Jan 14, 2013

Is dit een fix voor issue #16?
@Minnozz Het zou handig zijn als je aanpassingen in een aparte branch doet. Voordat je begint:
git checkout -b minozz-mijnaanpassing
Dat maakt mergen een stuk makkelijker.
Ik kijk er nu naar. Het lijkt in elk geval niks te breken (tot nu toe).

@aykevl
Copy link
Contributor

aykevl commented Jan 14, 2013

Wat mij betreft een verbetering. Een paar puntjes:

  • $displayname... $albumtitle klinkt wat mij betreft beter
  • In templates/footer.tpl staat nog een $humanname (dit soort dingen zijn snel te vinden met git grep. Of is er een reden dat je deze niet verandert hebt?)

$humanname is nog wel nodig in templates/actions.tpl.

@@ -121,10 +123,11 @@
template_assign('parentalbum');
}
template_assign('parentalbums');
template_assign('title', $humanname); // XXX
template_assign('title', $displayname); // XXX
Copy link
Contributor

Choose a reason for hiding this comment

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

Waarom staat daar nog steeds XXX? (Zie issue #16).

Copy link
Contributor

Choose a reason for hiding this comment

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

Omdat echte porno in Duitsland hosten blijkbaar onverstandig is ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Dus een htmlentities($displayname) zou genoeg zijn om die bug op te lossen?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ik weet echt niet meer waar die XXX voor was. Misschien inderdaad htmlentities..

Copy link
Contributor

Choose a reason for hiding this comment

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

Misschien weet @bwesterb dat wel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ik denk dat die XXX erover gaat dat het volledige pad naar de foto in de titel zou mogen staan, in plaats van alleen het huidige item (dus met album etc. erbij).
Daarnaast gaat het niet over htmlentities(), omdat dat in het template al gebeurt.

@Jille
Copy link
Contributor

Jille commented Jan 15, 2013

@aykevl93 Het maakt toch niks uit of hij het in een aparte branch doet of niet? master is ook gewoon een branch?

Code ziet er verder wel goed uit. Zodra ik tijd heb en de video-PR klaar is zal ik hem mergen.

@Minnozz
Copy link
Contributor Author

Minnozz commented Jan 15, 2013

@aykevl93: In de footer wordt inderdaad nog $pa['humanname'] gebruikt, met op die plaats een fallback: ?: $pa['name']. Het was inderdaad mooier geweest als die fallback in loadPathAlbums() had gezeten, maar dit doet hetzelfde.

@aykevl
Copy link
Contributor

aykevl commented Jan 15, 2013

@Jille Oke. Ik dacht dat dat gebruikelijk was.

@Minnozz Dat is waar. Ik had de fallback niet gezien.

@bwesterb
Copy link
Member

bwesterb commented Oct 6, 2013

@Jille ping

@Jille Jille removed their assignment Apr 7, 2015
@Minnozz Minnozz closed this Jun 23, 2017
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.

5 participants