Skip to content

Conversation

@brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Aug 13, 2018

  1. Change the editor field to a text field - I am sure you will never need a full wysiwyg editor here
  2. Add a label to the field called "Note"
  3. change the tmpl to get the list of fields from the xml instead of being hardcoded
  4. Add the content of the "note" to the list view. I really can't see the point in having the note if you can't see it in the list

(Same has been done for stages)

After

chrome_2018-08-13_17-23-24

chrome_2018-08-13_17-23-34

PR for joomla#21586

Despite comments the fix was NOT in joomla#21566
1. Change the editor field to a text field - I am sure you will never need a full wysiwyg editor here
2. Add a label to the field called "Note"
2. change the tmpl to get the list of fields from the xml instead of being hardcoded
3. Add the content of the "note" to the list view. I really can't see the point in having the note if you can't see it in the list
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Aug 13, 2018
</div>
<?php echo $this->form->getInput('description'); ?>
<?php foreach ($this->form->getGroup('transition') as $field) : ?>
<?php echo $field->renderField(); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

You've added some extra tabs here

@@ -1,9 +1,10 @@
<?xml version="1.0" encoding="utf-8"?>
<form>
<fields name="transition">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we indent the fieldset here if we're squeezing in the <fields> element please

@wilsonge
Copy link
Contributor

LGTM. Let's just get one test here

@brianteeman
Copy link
Contributor Author

Moved the name above the tabs for consistency

@brianteeman
Copy link
Contributor Author

Something isnt correct here - probably a silly error by me - but the edit is not displaying the note or saving correctly. I will take another look in the morning unless some kind sole wants to submit a pr to my branch with the fix

@wilsonge
Copy link
Contributor

You've added the fields tag. So the fields names have changed to e.g. jform[transition][description] instead of jform[description] before this PR. So you'll need to update the populateState function I would imagine

public function populateState()
{
parent::populateState();
$app = Factory::getApplication();
$context = $this->option . '.' . $this->name;
$extension = $app->getUserStateFromRequest($context . '.filter.extension', 'extension', 'com_content', 'cmd');
$this->setState('filter.extension', $extension);
}

(just doing this off code review - please check what i've written before attempting anything)

@brianteeman
Copy link
Contributor Author

Makes sense. Will check in the morning

@brianteeman
Copy link
Contributor Author

And probably the getState

@brianteeman brianteeman changed the title [4.0] Workflow Transitions [4.0] Workflow Transitions/Stages Aug 19, 2018
@brianteeman
Copy link
Contributor Author

OK now updated for stages and ready for testing

@brianteeman
Copy link
Contributor Author

@bembelimen I think this is clearer in the edit view and also means you can see all the notes at once in the list view - hope you agree

@bembelimen
Copy link
Contributor

In general I really like this PR, but two things:

  • perhaps move the note field to the right?
  • What is the reason for grouping the fields into the "transitions fields"? Will this be extended in some way, so we need the foreach in the tmpl? I would suggest to use fieldset only, so we have not this nested field naming George mentioned.

@brianteeman
Copy link
Contributor Author

perhaps move the note field to the right?

I assume you mean in the list view? It is below to be consistent with similar additional information on other lists - also allows a bit more space and if it was to wrap onto a second line it will look better this way

@wilsonge
Copy link
Contributor

What is the reason for grouping the fields into the "transitions fields"? Will this be extended in some way, so we need the foreach in the tmpl? I would suggest to use fieldset only, so we have not this nested field naming George mentioned.

We don't have nested fields anymore with this (that's when you use a "fields" tag) this just names the fieldsets, which is required for allowing plugins to add more custom fields (for example see what I've done with the groups view in #21696)

@bembelimen
Copy link
Contributor

bembelimen commented Aug 19, 2018

No I mean in the form view.

grafik

@bembelimen
Copy link
Contributor

@wilsonge But there is a difference: you add the name to the fieldset, here we added a new "fields" tag: https://github.com/joomla/joomla-cms/pull/21592/files#diff-6d3a9c00497a5503546ac43bbad8023eR10

@brianteeman
Copy link
Contributor Author

  1. Note - i think thats a just a matter of opinion - I dont care and am happy to move it
  2. Fields - my fault - didnt save the change - have pushed now

@brianteeman
Copy link
Contributor Author

conflicts resolved

<div class="small"><?php echo $item->description; ?></div>
<?php else: ?>
<?php echo $item->title; ?>
<div class="small"><?php echo $item->description; ?></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should add $this->escape(...) here, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was following your code style above for title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wilsonge please advise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@wilsonge wilsonge merged commit ed3db58 into joomla:4.0-dev Sep 10, 2018
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Sep 10, 2018
@brianteeman brianteeman deleted the monday-stage branch December 6, 2019 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants