Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/toolbar ui 2 #2350

Merged
merged 15 commits into from Feb 7, 2014
Merged

Feature/toolbar ui 2 #2350

merged 15 commits into from Feb 7, 2014

Conversation

@schmunk42
Copy link
Contributor

@schmunk42 schmunk42 commented Feb 6, 2014

continued from #2320 discussion...

  • @samdark the home icon could be handled just by a custom panel - maybe this will suit everyone?
  • instead of panelsPosition we could also introduce a position property
  • how to use   in the date formatter?
  • the toolbar on the debug module index view is a bit inconsistent with the other views
@schmunk42
Copy link
Contributor Author

@schmunk42 schmunk42 commented Feb 7, 2014

Will do some more iterations on this...

@@ -42,6 +42,10 @@ class Module extends \yii\base\Module
*/
public $panels = [];
/**
* @var string postion of the custom configured panels 'begin' or 'end'
*/
public $panelsPosition = 'end';

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

I don't think this is needed.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

What's about position then? Currently custom panels are appended, but I want them at to appear in custom order.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Is this really that important? Anyway I just added the support for disabling a core panel. You can now freely adjust the panel position as you want.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

Yes :) I'll add some screens to give you a better impression.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Well, then I would ask why couldn't my own panel be placed within core panels, and so on. Adding this option doesn't really solve problem.

@@ -59,7 +59,7 @@
'attribute' => 'time',
'value' => function ($data) use ($timeFormatter)
{
return $timeFormatter->asDateTime($data['time'], 'long');
return $timeFormatter->asDateTime($data['time'], 'H:i_j.M');

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

The original code is better.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

Then something else must be broken, because long outputs i.e. Friday201422 which is not a really readable date format :) I looked that up in the PHP manual, but found only the by-character replacements.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Yeah, use short then.

This comment has been minimized.

@Ragazzo

Ragazzo Feb 7, 2014
Contributor

Friday201422

what i18n locale are you using?

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

short gives: 05022014Fri, 07 Feb 2014 02:52:05 +010028 - there are no shorthands or am I missing an extension?

I suppose it's de, how can I find out btw?

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

That's definintely worth separate issue.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Ok, I think it's fine we don't use i18n date formatter here (since this is
for development purpose and other parts are not i18n'ed.)
We may just use simple format like yyyy/mm/dd hh:mm:ss

On Fri, Feb 7, 2014 at 8:13 AM, Alexander Makarov
notifications@github.comwrote:

In extensions/debug/views/default/index.php:

@@ -59,7 +59,7 @@
'attribute' => 'time',
'value' => function ($data) use ($timeFormatter)
{

  •           return $timeFormatter->asDateTime($data['time'], 'long');
    
  •           return $timeFormatter->asDateTime($data['time'], 'H:i_j.M');
    

That's definintely worth separate issue.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2350/files#r9537917
.

This comment has been minimized.

@Ragazzo

Ragazzo Feb 7, 2014
Contributor

right, so @schmunk42 can you please create separate issue for this case? it will be investigated and discussed there.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

Follow up issue #2359

This comment has been minimized.

@samdark

samdark Feb 7, 2014
Member

Thanks. I'll try to verify it during the weekened.

@@ -6,6 +6,9 @@
* @var yii\debug\panels\ConfigPanel $panel
*/
?>
<div class="yii-debug-toolbar-block title">
<?= Html::a('Yii Debugger', ['index'], ['title' => 'Back to main debug page']) ?>
</div>

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

It doesn't make sense to move the index block to the config panel. The original code is better.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

Will be reverted/update with the next push.

@schmunk42
Copy link
Contributor Author

@schmunk42 schmunk42 commented Feb 7, 2014

Some screens...

bildschirmfoto 2014-02-07 um 03 06 40


bildschirmfoto 2014-02-07 um 03 06 21


bildschirmfoto 2014-02-07 um 03 06 47


bildschirmfoto 2014-02-07 um 03 06 55


bildschirmfoto 2014-02-07 um 03 06 29

// load latest request
$tags = array_keys($this->getManifest());
$tag = reset($tags);
$this->loadData($tag);

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

It would cause confusion to show on the toolbar the latest debug data.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

But this is the only page were no data is shown, otherwise it's the current request or the request you debug at the moment. But yeah, could be removed.
But then the custom panels should be rendered before the core panels, otherwise this gets ugly and confusing.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Ok, we can show the latest data. But need to check if data is available.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

FYI, I just improved the Module::panels property so that you can specify the display order of core panels in whatever order you like.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

Just merged it, great!

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

Ok, we can show the latest data. But need to check if data is available.

@qiangxue There's already an exception in the loadData method.
throw new NotFoundHttpException("Unable to find debug data tagged with '$tag'.");

The problem is, when we don't define the panels for rendering we'd have to change the view also.

@@ -1,6 +1,4 @@
<div class="yii-debug-toolbar-block">
<a href="<?= $panel->getUrl() ?>" title="Total request processing time was <?= $time ?>">Time <span class="label"><?= $time ?></span></a>
</div>
<div class="yii-debug-toolbar-block">

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Better separate them in two blocks since they are two links.

This comment has been minimized.

@schmunk42

schmunk42 Feb 7, 2014
Author Contributor

It's the same link. Logically speaking the whole block is a link.
That's one thing which really confused me in the beginning.

We are already using css-labels to separate the description and information and block borders for links.

This comment has been minimized.

@qiangxue

qiangxue Feb 7, 2014
Member

Make sense.

@schmunk42 schmunk42 mentioned this pull request Feb 7, 2014
0 of 1 task complete
qiangxue added a commit that referenced this pull request Feb 7, 2014
@qiangxue qiangxue merged commit 20fdbc3 into yiisoft:master Feb 7, 2014
1 check failed
1 check failed
default Scrutinizer: 1 new/changed issues, 2 added/modified code elements — Travis: Errored
Details
@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 7, 2014

Thanks!

@cebe cebe added this to the 2.0 Beta milestone Feb 7, 2014
@schmunk42
Copy link
Contributor Author

@schmunk42 schmunk42 commented Feb 7, 2014

@qiangxue @samdark @Ragazzo One more thing :)

I would like to make the logo and title configurable via module configuration - any objections?
Then it would be absolutely perfect!

Another small issue is, that the panels request and db are mandatory at the moment, but that can be fixed I think.

Thanks also for merging so far, I am really looking forward seeing developers providing custom panels for their extensions.

@samdark
Copy link
Member

@samdark samdark commented Feb 7, 2014

No objections.

@Ragazzo
Copy link
Contributor

@Ragazzo Ragazzo commented Feb 7, 2014

I would like to make the logo and title configurable via module configuration

i am not sure with this one, its fine as is. you already can override this methods if needed.

I ok with the rest.

@qiangxue
Copy link
Member

@qiangxue qiangxue commented Feb 7, 2014

Another small issue is, that the panels request and db are mandatory at the moment, but that can be fixed I think.

Yes, this should be fixed since we now allow disabling any core panel.

@schmunk42 schmunk42 deleted the schmunk42:feature/toolbar-ui-2 branch Mar 17, 2014
@schmunk42 schmunk42 mentioned this pull request Mar 24, 2014
0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.