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

[5.1] Adding notice to global configuration for additional options in SEF plugin #42832

Merged
merged 7 commits into from Feb 28, 2024

Conversation

LadySolveig
Copy link
Contributor

Pull Request for PR #42692 #42702 #42704 .

Summary of Changes

Add note to gloabl configuration for additional options in system plugin SEF

grafik

Testing Instructions

  • Apply patch
  • open global configuration /administrator/index.php?option=com_config

Actual result BEFORE applying this Pull Request

no notice shown

Expected result AFTER applying this Pull Request

notice shown in section/fieldset "SEO"

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Co-authored-by: Quy <quy@nomonkeybiz.com>
@Hackwar
Copy link
Member

Hackwar commented Feb 19, 2024

I have tested this item ✅ successfully on e42c9d5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42832.

@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

Sorry, this looks wrong, the layout already have "description" rendering,

<?php if (!empty($displayData->description)) : ?>
<p><?php echo $displayData->description; ?></p>
<?php endif; ?>

Need to look why it does not work as expected.

@Hackwar
Copy link
Member

Hackwar commented Feb 19, 2024

No, this is a different description. $displayData->description is not filled from the XML. To be honest, the old code looks like it was never tested.

@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

Then we have to fix that, instead of hardcoding.

From what I see the description already expected from $displayData->description
in same way as label done with $displayData->name

@Hackwar
Copy link
Member

Hackwar commented Feb 19, 2024

The fix is this PR here. $displayData->description is simply wrong.

@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

Why?

@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

The description need to be added here

$this->name = Text::_('COM_CONFIG_SEO_SETTINGS');

Beside $this->name:

$this->name = 'blabla name'
$this->description = 'blabla description';

But, this also means that every other layouts in ../tmpl/application should have "description":

$this->name = 'blabla name'
$this->description = '';

Otherwise, the last added description will be rendered in all following layouts.

@Hackwar
Copy link
Member

Hackwar commented Feb 19, 2024

Yes, and that is why this shouldn't be done like that. The whole layout obviously has only been created for the global configuration and has not been maintained properly so far. For example, the whole rendering of fields can be replaced with echo $displayData->form->renderFieldset($fieldname);.

Co-authored-by: Quy <quy@nomonkeybiz.com>
@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

Yes, and that is why this shouldn't be done like that. The whole layout obviously has only been created for the global configuration and has not been maintained properly so far.

Sorry, I cannot follow your logic.
We better stick to what we have here instead of hardcoding here and there.

If someone want to rewrite whole rendering, well, I do not mind. But then it should be done for whole view, not only here.

@LadySolveig
Copy link
Contributor Author

@Fedik
#42692 (comment)
I have now simply taken up this approach for a different proposal.

<?php if (!empty($fieldSet->description)) : ?>

@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

I understood that, but as you can see application view rendred diferently from component view.
I do not know why, but unless someone want to rewrite it, it is better to follow of what it is.

To have a better styling can probably change

<?php if (!empty($displayData->description)) : ?>
<p><?php echo $displayData->description; ?></p>
<?php endif; ?>

to:

<?php if (!empty($displayData->description)) : ?>
  <div class="alert alert-info">
    <span class="icon-info-circle" aria-hidden="true"></span><span class="visually-hidden"><?php echo Text::_('INFO'); ?></span>
    <?php echo $displayData->description; ?>
  </div>
<?php endif; ?>

Similar to what you made, but with use of 'expected' description property.

@Fedik
Copy link
Member

Fedik commented Feb 19, 2024

@LadySolveig looks good now, thanks!

@ceford
Copy link
Contributor

ceford commented Feb 20, 2024

I have tested this item ✅ successfully on e8f9963


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42832.

1 similar comment
@Quy
Copy link
Contributor

Quy commented Feb 20, 2024

I have tested this item ✅ successfully on e8f9963


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42832.

@Quy
Copy link
Contributor

Quy commented Feb 20, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42832.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 20, 2024
@crimle
Copy link

crimle commented Feb 24, 2024

I have tested this item ✅ successfully on c9f426a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42832.

1 similar comment
@dorisdreher
Copy link

I have tested this item ✅ successfully on c9f426a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42832.

@bembelimen bembelimen merged commit 96618f6 into joomla:5.1-dev Feb 28, 2024
0 of 2 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 28, 2024
@bembelimen
Copy link
Contributor

Thank you!

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 PR-5.1-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants