Fix: Use alternative syntax in view scripts #384
Conversation
| <?php $socialSignIn = true; ?> | ||
| <?php foreach ($this->options->getEnabledProviders() as $provider): ?> | ||
| <?php if ($socialSignIn): ?> | ||
| <?php $socialSignIn = false; ?> |
There was a problem hiding this comment.
someone know for this var is used?
There was a problem hiding this comment.
Maybe some
?
Removing it in a second!
There was a problem hiding this comment.
Looks like this was mostly copied from
<?php
$socialSignIn = true;
foreach ($this->options->getEnabledProviders() as $provider) {
if ($socialSignIn) {
echo '<h1>Social Sign In</h1>';
$socialSignIn = false;
}
echo '<dd>' . $this->socialSignInButton($provider, $this->redirect) . '</dd>';
}
if ($this->options->getSocialLoginOnly() == false) {
echo $this->zfcUserLogin;
}
|
Thank you for reviewing, implemented the suggested changes! |
localheinz
force-pushed
the
fix/alternative-syntax-in-view-scripts
branch
from
aea0663
to
b8b481a
Compare
|
This requires a rebase |
localheinz
force-pushed
the
fix/alternative-syntax-in-view-scripts
branch
from
cc0f1f1
to
facbce4
Compare
|
Rebased! Thanks a lot for reviewing everything! |
| <div class="alert alert-block">No modules have been submitted yet</div> | ||
| <?php else: ?> | ||
| <?php foreach ($modules as $module): ?> | ||
| <?php echo $this->moduleView([ |
There was a problem hiding this comment.
Why does this helper require so many parameters? Isn't it easier to pass in $module instead?
There was a problem hiding this comment.
That's what I plan to resolve next! 😉
| @@ -11,7 +11,7 @@ | |||
| </div> | |||
| </div> | |||
| <?php foreach ($this->flashMessenger()->getMessages() as $message): ?> | |||
| <h3 class="zf-green"><?php echo $message ?></h3> | |||
| <h3 class="zf-green"><?php echo $this->escapeHtml($message); ?></h3> | |||
There was a problem hiding this comment.
Escaping for now, would like to re-iterate on this and use the FlashMessenger directly, as suggested!
Also, I would like to use http://getbootstrap.com/components/#alerts, what do you think?
There was a problem hiding this comment.
Can be done in a different MR then (with screenshots 'n stuff, so someone with better taste than me can deal with it :-) )
There was a problem hiding this comment.
would like to do this if @localheinz agrees - as i allready implemented this on a diff project (personal) 😼 :
There was a problem hiding this comment.
Sure, go ahead!
There was a problem hiding this comment.
If you do, please have a look at places where classes are missing:
<div class="alert alert-block">No modules found</div>
which results in
vs.
<div class="alert alert-info alert-block">No modules found</div>
which would result in
💡 Just as an idea, in Getting Real from 37signals, there's a chapter The Blank State - I think we could improve on that.
…w-scripts Fix: Use alternative syntax in view scripts
This PR