feat: create config to display website icon only / name only / icon and name / none on feeds

[sad270]

Closes #4838

Hello, I create this PR in draft, I'm open to review (codestyle, naming, other idea), it's my first real PR (with real code).

To be consistent with existing code for the topline_thumbnail (I copy it). I didn't use constants for full, none, name and icon values, but IMHO we should use constant for both config. If you are ok with this I can use constant for website and do the refactor for the thumbnail.

I want to create a enum for the values, but it's not available in PHP 7.

I don't know how translation works, I copy/past for all language (in no brain mode !). I see some po/pot files in project, maybe there is a translation tools ? if not I can add french translation in this PR directly.

I didn't run tests and quality tools, my dev env is not working, I will try again later.

Changes proposed in this pull request:

• Create a new display setting where you can select to display icon only or name only or icon and name or none for website on the feeds view

How to test the feature manually:

1. Open the homepage localhost:8080/i/ and check that website icon and label are visible by default.
2. Go to settings, check the new Website settings is here.
3. Change the website value and save.
4. Go back to homepage to verify that website display has changed.

Screenshots

The settings view :

Feed in full view (default) :

Feed in none view :

Feed in icon view :

Feed in name view :

Pull request checklist:

• clear commit messages
• code manually tested
• unit tests written (optional if too hard)
• documentation updated

Additional information can be found in the documentation.

[sad270]

I rebase on edge and add some translation.
I see for using constant. It's a little bit difficult. The best think to do IMO, is to use a ValueObject, and store the availlable choices in this ValueObject (as constant).

I want to add a WebsiteConfiguration object in Models/Configurations (and step by step do the same for all configurations), but I need to change the autoloader. IMO this is too big for this MR, in must be done (if we want to do this) in another MR.

The autoloader is weired, why you don't use namespaces ? and why you don't use composer for no-dev dependencies ?

[math-GH]

There is an issue with the title hover:

because of the max-width, that calculates with the width of the website column

[Frenzie]

The CSS in your screenshot doesn't seem to make much sense. Looks like it should just be removed completely?

[math-GH]

The CSS in your screenshot doesn't seem to make much sense. Looks like it should just be removed completely?

It is needed to cut the title. If the title will not be cut than it will overflow and a x-axis scroll bar will appear

[Frenzie]

I said the CSS, not the max-width. But okay, so the intended behavior on the current default style is as follows:

(top not hovered, bottom hovered)

[math-GH]

I said the CSS, not the max-width. But okay, .....

Please help me to understand you. Which CSS does not make sense?

[Frenzie]

Given the context that was available at the time, the entire thing.

[math-GH]

I have an idea:
I would add a CSS class (f.e. noWebsite, onlyIcon, onlyWebsite) to <ul class="horizontal-list flux_header"> and would solve it with CSS instead of manipulation of the DOM in .phtml. Then CSS could solve the :hover .item.title issue.

@sad270 If you would allow I could support here with commits

[sad270]

I have an idea:
I would add a CSS class (f.e. noWebsite, onlyIcon, onlyWebsite) to <ul class="horizontal-list flux_header"> and would solve it with CSS instead of manipulation of the DOM in .phtml. Then CSS could solve the :hover .item.title issue.

@sad270 If you would allow I could support here with commits

It's open source, and my PR is open source too 😁 feels free if you want to contribute.

I already add a css class on the 'item website' element with the selected option value if it can help.
If not, maybe remove the classname I add on 'item website' add it on the 'ul' element as you said. And use this new classes ?

[math-GH]

I made a commit. Please check.

[sad270]

@math-GH I'm sorry I think I didn't understand what you said. I understand that you want to remove the date on hover, so you need to add the config in a parent element to be able to apply css rules on other column.
I don't think that was a good idea to manage the displaying of icon / website name in the CSS. Why should we add the website name and the favicon when not needed to display ?

I don't understand your problem. Why don't you remove the CSS ? If I remove max-width and position: absolute rules like this.

.flux:not(.current):hover .item.title {
	background-color: inherit;
}

Everything works good for me. And they seems to be useless ? Maybe legacy rules ?

Note: I will update and force push my branch. I will rebase your commit and mine to have them after the commit from edge branch. And fix type in french translation ;)

[Frenzie]

Everything works good for me. And they seems to be useless ? Maybe legacy rules ?

The default display is single line. See #4969 (comment)

I greatly prefer multiline but in any case it's not useless at this time. How much added value there is to showing a mere handful of extra chars is certainly debatable though, and I'd also rather just remove those lines.

[math-GH]

I don't think that was a good idea to manage the displaying of icon / website name in the CSS. Why should we add the website name and the favicon when not needed to display ?

You are right. I reverted it

[sad270]

@Frenzie how do you do to have the view in your comment ? Do you add custom css ? I try différents config' with différents width of screen ? I didn't have your the same behavour you have in your screen.

[sad270]

@Frenzie up ?

[Frenzie]

While I do use custom CSS, I use it to make it look like this:

The behavior I showed in the screenshots is the default.

[sad270]

@Frenzie I didn't have this behavour by default => #4969 (comment)
@math-GH I didn't have this behavour by default => #4969 (comment)

How did you do (what is your settings) to have this ? If it is custom css, send the custom css you added to have this. I will try to reproduice this and help you to fix it. I didn't understand if my code isn't good or the custom CSS may be updated.

[Frenzie]

The behavior in #4969 (comment) is the default, not custom. Perhaps you mean you don't have it due to the changes in this PR?

[Alkarex]

@Frenzie and @math-GH : Could you please help me with a quick status here. Do you still have any pending suggestion / comment ?

[Frenzie]

I don't mind changing the behavior if it's a conscious decision.

[math-GH]

As far as I can oversee it is okay now