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
Mobile-view: paragraph section fixes #8492
Mobile-view: paragraph section fixes #8492
Conversation
Darshan-upadhyay1110
commented
Mar 8, 2024
- Fix image path for base spin field icons
- Do not hide or show elements which are force to be hidden in mobile view
- image path was wrongly assigned - setImage method will do the work to set image path Signed-off-by: Darshan-upadhyay1110 <darshan.upadhyay@collabora.com> Change-Id: I765f0049d7d607926529df38a202b9ec923584b1
/me sees this is still a draft |
var headers = currentNode.siblings().filter(function() { | ||
return $(this).attr('style') === 'display: none;'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this ok
if ($(this).attr('style') === 'display: none;') { | ||
headers = headers.add($(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok? Meaning, will it always work? wouldn't be better to $(this).style.display === 'none'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(this).style.display === 'none'
will not work as it consider all the elements which have display = none ( including the elements which we forcefully hide by css )
this condition will only consider elements which have inline property display = none
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i am seeing you are right if in future there are additional inline css line were added then my condition will fail 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedropintosilva $(this).attr('style').includes('display: none')
will work, let me test...
- We specifically hide some section or elements in mobile view - if we go level up or level down in mobile view, we should not change the state of those elements which already hidden using css rules - this patch will cover that part of issue where we do not consider elements which are hidden by css rules Signed-off-by: Darshan-upadhyay1110 <darshan.upadhyay@collabora.com> Change-Id: I7ffd83c5b74987f74b7e327fd13775a81d02120d
9bfe45f
to
e4079f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but it would be nice to wait and see if #8517 passes the CI also could you give an example of
Do not hide or show elements which are force to be hidden in mobile view
/* Paragraph properties */ https://github.com/CollaboraOnline/online/blob/master/browser/css/mobilewizard.css#L1234 Ref commit where we did the hide thing : 7679958 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok