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.0] Add toolbar.scss in Cassiopeia #33403

Merged
merged 1 commit into from
May 9, 2021
Merged

[4.0] Add toolbar.scss in Cassiopeia #33403

merged 1 commit into from
May 9, 2021

Conversation

rjharishabh
Copy link
Contributor

Pull Request for Issue #33392.

Summary of Changes

Replicate Atum toolbar in Cassiopeia

Testing Instructions

  • Log In to the frontend
  • Go to Template Setting in Author Menu module or Visit http://localhost/joomla-cms/index.php/create-a-post/template-settings
  • Click Select
  • Apply PR
  • Do npm run build:css
  • Refresh the page and Click Select
  • See the difference

Actual result BEFORE applying this Pull Request

subhead noshadow missing CSS

before-subhead

Expected result AFTER applying this Pull Request

Added CSS in subhead noshadow

after-subhead

Documentation Changes Required

None

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Apr 29, 2021
@ghost
Copy link

ghost commented Apr 29, 2021

I have tested this item ✅ successfully on 59b1cce


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

@rjharishabh
Copy link
Contributor Author

@Quy Code review please

@Quy
Copy link
Contributor

Quy commented Apr 30, 2021

Sorry I am not at my computer to test. I will do tomorrow.

@rjharishabh
Copy link
Contributor Author

Sorry I am not at my computer to test. I will do tomorrow.

No problem

@richard67
Copy link
Member

I have tested this item ✅ successfully on 59b1cce


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

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 1, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 1, 2021
@bembelimen
Copy link
Contributor

Thank you for this PR @rjharishabh
TBH I'm not 100% happy with the approach to "hard enforce" things like: https://github.com/joomla/joomla-cms/pull/33403/files#diff-e1218a0d5d75ad354fd21362d37ab7fd8bf7731253e4a6daa09c5bb34a10afd0R14-R17 and have a mix of PX + REM. The frontend template is BS5 anyways, so is there no way to go with the frontend styling?

@rjharishabh
Copy link
Contributor Author

@bembelimen I want to change PX to REM
but there is no direct relation to converting px to rem, it depends on the default pixel size
I replicate Atum toolbar code to Cassiopeia, then I will change there also

@bembelimen
Copy link
Contributor

Default is: 1rem == 16px

@bembelimen
Copy link
Contributor

If you use the same toolbar anyways, you can ofc import it directly instead having two versions...

@brianteeman
Copy link
Contributor

@bembelimen you can't import it because there are atum only variables

@richard67 richard67 removed the RTC This Pull Request is Ready To Commit label May 6, 2021
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone May 6, 2021
@richard67
Copy link
Member

Back to pending due to changes requested in comments above.


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

@richard67 richard67 added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label May 6, 2021
@rjharishabh
Copy link
Contributor Author

@bembelimen you can't import it because there are atum only variables

Yes, there are atum specific variables like atum-bg-dark , atum-link-color, atum-special-color

@brianteeman
Copy link
Contributor

@bembelimen why are you asking for the rem to be converted to px? This file was identical to the one in atum except for the color variables. I'm wondering what the logic is for that request.

The link you posted as an example just opens the entire file so its not clear if you really meant the entire file or just some specific parts.

/me confused and can't see anything wrong with the original pr

@bembelimen
Copy link
Contributor

Sorry my fault, the link was only for the ".row" because that's a default element exactly for adding the margin. So it hurts a bit my guts to remove the margin that way, just wanted to point at it as example for "hard enforce".

The px => rem thing was independent from it another issue. We should not mix it I think but have rem when not a 1px border.

@brianteeman
Copy link
Contributor

I understand you now. However as its just the same in atum I wouldnt bother to change it.

the px/rem thing is beyond my skillset. Again its just a direct copy from atum and to be honest seeing a value of .313 is just as ugly

@rjharishabh
Copy link
Contributor Author

rjharishabh commented May 6, 2021

to be honest seeing a value of .313 is just as ugly

Same with me

Now I understand @bembelimen @brianteeman

It's my mistake to understand it in a wrong way

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 59b1cce


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

@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label May 6, 2021
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 7, 2021
@richard67 richard67 added this to the Joomla 4.0 milestone May 7, 2021
@richard67 richard67 merged commit 9c6d53a into joomla:4.0-dev May 9, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 9, 2021
@richard67
Copy link
Member

Thanks!

@rjharishabh
Copy link
Contributor Author

Thanks for merging

@rjharishabh rjharishabh deleted the toolbar branch May 9, 2021 17:54
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Jul 19, 2021
This css was added in error with joomla#33403

It is not needed in the front end as this is used only for the mobile admin ui toggler seen below
richard67 pushed a commit that referenced this pull request Jul 21, 2021
This css was added in error with #33403

It is not needed in the front end as this is used only for the mobile admin ui toggler seen below
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

6 participants