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

[4] New moduleposition below-top, option to switch off logo #33751

Merged
merged 7 commits into from May 14, 2021

Conversation

chmst
Copy link
Contributor

@chmst chmst commented May 10, 2021

Summary of Changes

Not all users want it have a logo in the header of their site. Cassipoeia has no option to switch off the display of a logo or description
This PR adds a param to the template where the user can switch off the logo.
It adds a new position "below-top" where users can add an own module.
Using both - position "below-top" and logo is up to the user.

The error page can be adapted if this is accepted.

Testing Instructions

Apply the patch and play with the template style cassiopeia. Use the module position brand

Actual result BEFORE applying this Pull Request

There is always an area for the logo above the navigation.

Expected result AFTER applying this Pull Request

The user can swith off the logo
grafik

Documentation Changes Required

yes

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels May 10, 2021
@brianteeman
Copy link
Contributor

brianteeman commented May 10, 2021

It is confusing having a module and an options with the same name that control different things.
The way it is used here in the options it is not clear what it does unless you read the code
Is it intentional that you can have two brands?

I think you need to rethink the approach here.

@chmst
Copy link
Contributor Author

chmst commented May 10, 2021

Have you a suggestion for a name of the module position? Maybe below-top?


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

@brianteeman
Copy link
Contributor

Thinking out aloud.

How about instead of saying Brand Yes|No maybe Template|Module would make more sense?

@chmst
Copy link
Contributor Author

chmst commented May 10, 2021

I agree that the name brand for a moduleposition is wrong, as the position canbe used for everything. It is independent from
displaying the brand ( = Logo / Description and Tagline ).

@ghost
Copy link

ghost commented May 11, 2021

I have tested this item ✅ successfully on dba8af8

Similar position in github named "Header".


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

@chmst chmst changed the title [4] New moduleposition brand, option to switch off logo [4] New moduleposition below-top, option to switch off logo May 11, 2021
@chmst
Copy link
Contributor Author

chmst commented May 11, 2021

I have changed the name, thanks for the hint.

The position below-top can be be used independent form the brand ( logo / description + tagliene).

grafik

@ghost
Copy link

ghost commented May 12, 2021

I have tested this item ✅ successfully on 7894923

1. Setting in template style:

Screen Shot 2021-05-12 at 13 05 24

2. Frontend:

Screen Shot 2021-05-12 at 13 08 49

3. Frontend + Language switcher in position "below-top":

Screen Shot 2021-05-12 at 13 09 23


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

@ChristineWk
Copy link

I have tested this item ✅ successfully on 7894923


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

@richard67
Copy link
Member

@chmst Please add the missing language string for the new module position to file language/en-GB/tpl_cassiopeia.sys.ini so that people see a nice module position name in the pulldown, like we have it e.g. for the "Menu" position, where we see "Menu" and not "menu".

The name of the language string has to be: TPL_CASSIOPEIA_POSITION_BELOW-TOP.

@chmst
Copy link
Contributor Author

chmst commented May 12, 2021

Language string added, Thanks @richard67

@richard67
Copy link
Member

Language string added, Thanks @richard67

As far as I know, the name of the language string after the last underscore has to be equal to the name in the XML converted to uppercase.

That means it should be TPL_CASSIOPEIA_POSITION_BELOW-TOP.

But you have added TPL_CASSIOPEIA_POSITION_BELOW_TOP.

Just below you can see for bottom-a and bottom-b how it shoud be.

@richard67
Copy link
Member

@sandramay0905 @ChristineWk Could you briefly test this PR again? It has received a change so that when you select the new module position for a module, you see the nice, translatable language string "Below Top" in the dropdown, instead of the raw "below-top" from the XML file. Just test this new change, the rest hasn't changed. Thanks in advance, and thanks so far for all previous testing here and elsewhere.

@ChristineWk
Copy link

Should be OK:

screen shot 2021-05-12 at 20 33 34


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

@ChristineWk
Copy link

I have tested this item ✅ successfully on 231cc0f


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

@richard67
Copy link
Member

@ChristineWk What I meant with the changed thing to be tested was the name of the module position in backend, when you select the position:

2021-05-12_2

Before the last change, it was just "below-top", or maybe "below-top [below-top]", now it has the "Below Top" at the beginning, which is a translatable text.

@ChristineWk
Copy link

Ah, OK :-)
screen shot 2021-05-12 at 20 51 20


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

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 231cc0f


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

@brianteeman
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 231cc0f


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

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 231cc0f

sorry this is a good test


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 12, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 12, 2021
@rdeutz rdeutz merged commit 00cc7c4 into joomla:4.0-dev May 14, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 14, 2021
@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Contributor

Shouldnt the param have been set in the database on install? (it hasnt been and therefore the problem)

@richard67
Copy link
Member

@brianteeman @PhilETaylor I'll make a PR soon.

@richard67
Copy link
Member

@brianteeman @PhilETaylor PR #33881 is ready, please test. Thanks in advance.

@infograf768
Copy link
Member

infograf768 commented May 15, 2021

While I was working on #31275 I discovered that I had issues when using the lang switcher with flags as a dropdown in the new position.

switcher_top_position

I will have to add the color in the module css

div.mod-languages .btn-group .btn {
    flex: none;
    color: #000;
}

@richard67
Copy link
Member

@brianteeman @PhilETaylor I've meanwhile replace my PR by a new, better one, for the issue with the header section not shown after a new installation or an update before having saved template style parameters. Please test #33930 . Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Template Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants