Update Case Contacts Section-1 #5366
Conversation
chahmedejaz
requested review from
compwron,
FireLemons and
littleforest
as code owners
public/assets/css/main.css
Outdated
| @@ -54,9 +54,6 @@ i, | |||
| a { | |||
| display: inline-block; } | |||
|
|
|||
| span { | |||
| display: block; } | |||
| } | |||
There was a problem hiding this comment.
This was a syntax error in the main.css. Moreover, this doesn't make sense to change the default inline behavior for the <span> tag here to block. Doesn't it kill the purpose of span?
There was a problem hiding this comment.
I’m so confused how this error has been here since February…? But definitely seems like there is an extra curly brace. Did you poke around the pages and make sure this removal didn’t cause any major styling issues?
There was a problem hiding this comment.
Really sorry my ignorance here, @littleforest.
- I was at the thought that this span style was being used only for the asterisk signs for them to be on the new line
- This fact skipped my mind that it's a global style I'm changing 😓
- I validated now and found many styling issues due to this. 😢
- Anyway, I'll add a custom CSS class to override this style for the required asterisk signs here.
- However, I believe we should have an enhancement issue where this style should be removed and all the styling issues should be handled accordingly as well.
- By nature
spanis an inline element and changing this nature toblockglobally kills the purpose of this element. - We have other alternatives to achieve this behavior like paragraph tag, divs etc..
- What do you say?
There was a problem hiding this comment.
@chahmedejaz I agree that it is odd that span is set to display: block. Do you want to create a new ticket and see about making that change? You will probably have to touch a lot of pages/styles.
There was a problem hiding this comment.
@chahmedejaz I agree that it is odd that span is set to display: block. Do you want to create a new ticket and see about making that change? You will probably have to touch a lot of pages/styles.
Sure, I'd love to work on this refactor in a new ticket. I'll create another ticket will work on that as well. Thanks :)
There was a problem hiding this comment.
@littleforest - The respective ticket to handle this has been created:
#5377
Please review and let me know if something is missing. Thanks.
chahmedejaz
force-pushed
the
task/5347-update-case-contacts-section1-styles
branch
from
3b778a1 to
4470db2
Compare
|
@littleforest - I've addressed your comment. The PR is ready for review now. Thanks. |
|
@chahmedejaz can you fix merge conflicts? Looks good to me otherwise. |
It's fixed now @littleforest. You can move forward with this. |
What GitHub issue is this PR for, if any?
Resolves #5347
What changed, and why?
main.css, we are overriding theinlinebehavior of the span tag toblockwhich kills its purpose. Hence remove thisas wellHow will this affect user permissions?
How is this tested? (please write tests!) 💖💪
Screenshots :)