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

[6.0] Gsoc2021 35228 merge featured #43907

Open
wants to merge 32 commits into
base: 6.0-dev
Choose a base branch
from

Conversation

chmst
Copy link
Contributor

@chmst chmst commented Aug 11, 2024

Pull Request for Issue # .
This Pull request rewrites the #35228 as meanwhile many changes were made.

The purpose of theis PR is to merge the "featured" articles into the articles classes and remove duplicate code from com_content
This PR changes only the backend, nothing in frontend.

Summary of Changes

See PR #35228
Additionally the quickicon "featured".

Testing Instructions

as described in #35228, plus a view on quickicons

If you test on an existing site, please do this as first step:

UPDATE #__menu
SET link = 'index.php?option=com_content&view=articles&featured=1'
WHERE link = 'index.php?option=com_content&view=featured'
AND client_id = 1;

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

See PR #35228

Link to documentations

Please select:

  • Documentation link for docs.joomla.org: help doc and tbd

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org: tbd

  • No documentation changes for manual.joomla.org needed

@richard67
Copy link
Member

@chmst There are still code style issues, e.g. unused imports (use statements) to be removed. Unfortunately many of them are outside of what can be done with a code change suggestion on GitHub, so I can't help with suggestions here. See the log here: https://ci.joomla.org/joomla/joomla-cms/78000/1/7 . If necessary I can help tomorrow after work. Would be good to get the phpcs fixed so the system tests will be started in Drone.

@brianteeman
Copy link
Contributor

I thought the policy was deprecated in x and removed in x+2 ?

@richard67
Copy link
Member

@chmst Does this PR replace PR #35228 ? If yes, shall we close the latter?

And could you check the conflicting files? I think the conflicts come from PRs #43323 and #43512 , but I don't have the time now to check in detail.

@richard67
Copy link
Member

@chmst I've allowed myself to fix the conflicts. Please check the result.

@fgsw
Copy link

fgsw commented Aug 21, 2024

  1. Full prebuilt package installation.
  2. Blog sample data installation failed (red There is an error in a sample data plugin. Response is invalid.)
  3. Saving new Article:
    UntitledArticle not saved.

@obuisard
Copy link
Contributor

The PR has the support of the Enhancement Development team. This is still something the team wants to talk to the maintenance team and the release managers for 6.0 about: we do think it would make future changes to the core simpler if this PR was merged early on and tested throughout the 6.0 pre-release cycle.

@brianteeman
Copy link
Contributor

@obuisard thats what is always said and the fixes never happen. Once it is merged then the people behind it move on to other things.

@obuisard
Copy link
Contributor

Adding the following code in the DisplayController will prevent old URLs to fail with 500 error but be redirected to the 'articles' view instead.

if ($view == 'featured') {
    $this->setRedirect(Route::_('index.php?option=com_content&view=articles&featured=1', false));
    return false;
}

@chmst
Copy link
Contributor Author

chmst commented Sep 24, 2024

Just started a new Installation. I think that the first select "All" is not self-explaining.
grafik

Would "featured / unfeatured " be better? The text is rather long especially in other languages.
or " - select featured - " ?

@richard67
Copy link
Member

@chmst Could you check the conflict in file „administrator/components/com_content/tmpl/featured/default.php“? I think it comes from #44053 .

@obuisard
Copy link
Contributor

Just started a new Installation. I think that the first select "All" is not self-explaining.
Would "featured / unfeatured " be better? The text is rather long especially in other languages. or " - select featured - " ?

In my opinion, " - select featured - ", "featured", "unfeatured" sounds better.

# Conflicts:
#	administrator/components/com_content/forms/filter_featured.xml
#	administrator/components/com_content/src/Model/FeaturedModel.php
#	administrator/components/com_content/src/View/Articles/HtmlView.php
#	administrator/components/com_content/src/View/Featured/HtmlView.php
#	administrator/components/com_content/tmpl/featured/default.php
@chmst
Copy link
Contributor Author

chmst commented Dec 14, 2024

The filter needs to be changed. It cannot be featured / unfeatured but also needs a value "all".

-- select featured --
all
featured
unfeatured

@chmst chmst changed the title [6.0] [WIP] Gsoc2021 35228 merge featured [6.0] Gsoc2021 35228 merge featured Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants