-
Notifications
You must be signed in to change notification settings - Fork 0
Drale1/6.x/#526 jquery free and implement dxb slider #529
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
Drale1/6.x/#526 jquery free and implement dxb slider #529
Conversation
…de in dxpr-theme-settings.admin.js for that
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.
It looks like there are some automatic formatting changes in this file. All changes that are not directly related to a code change should be kept as is.
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.
@drale1 I believe this looks good overall, nice work! I only had a couple of questions, and I would also need to do a more thorough test before I approve. Otherwise there was only the formatting issues for now.
EDIT: Also please resolve the merge conflicts, thanks!
|
Live slider changing works. I'll work over weekend to fix it. |
|
I’ve fixed the saving issue by adding $form['#attributes']['novalidate'] = 'novalidate';. I'm not entirely sure if this is the best solution, but the issue occurs when trying to save decimal values. It might be related to how DXB-slider handles decimals. Additionally, I’ve fixed the correct flex display in the Typography section. |
|
I will review it when I review it :) @drale1 |
|
I fixed eslint issue. I removed live dynamic change on: And also on this sliders: Value can't be change by using the keyboard up/down or click up/down with the mouse. |
jjroelofs
left a comment
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.
@drale1 Great work Drazen, from a quick glance I noticed the header opacity and sticky header background opacity settings are not working.
Because there are so many changes now let's please go back the the process where @deuxcode does in-depth review, and after his final review passes you request my review again. thx.
deuxcode
left a comment
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.
@drale1 I've reviewed and found a few issues that I didn't see before. Let me know if you need any more details on any of them.
Needs to be fixed
- The "Block Design Preset" field is force to "Custom" value, and I'm not able to change it to anything else. It just jumps back to Custom.
- Block Design > Divider Element - The three "Customize Divider" input fields are visible even with the checkbox unchecked. These should be hidden and only visible then "Customize Divider" is checked.
This also happens to the Layout > Box max-width when the "Boxed layout" is unchecked, i.e. the input field should be hidden when unchecked. - Sticky Header Height can go up to 2090. I don't think that was the case before. I believe it was 200, just as the non-sticky header.
- Header Height affects the sticky header height. This should only affect the non-sticky header, as the Sticky header has its own Height slider.
Middle of the road - Jurrian's choice
- Container Space - This field gets erratic on larger values, similar to the other max-width ones. Not sure why large values are needed here. If set to max 200 this works a bit better.
- The eye-icon
.no-preview::aftershould be a bit smaller I think, like 1 rem.
Can be ignored but wanted to point out
- Unable to edit the values directly, and it's not possible to use up/down arrows on input field or up/down arrow keys. It's essentially a readonly field. I don't think it's a blocker though.
…ck Advanced section
On my side, the header opacity and sticky header background opacity settings work exactly the same as before I started working. I can record a screen for an explanation if necessary.
Fixed the issue where the "Block Design Preset" field was forced to "Custom" value, and it could not be changed to anything else.
Fixed the visibility issue with the "Block Design > Divider Element" and "Layout > Box max-width" fields. The fields are now hidden when the respective checkboxes are unchecked.
My mistake. It is fixed to 200 max now.
On my side, both the "Header Height" and "Sticky Header Height" sliders work independently. I can also record a screen to explain if needed. |
|
"On my side, the header opacity and sticky header background opacity settings work exactly the same as before I started working. I can record a screen for an explanation if necessary." |
https://www.youtube.com/watch?v=0-ADj0_CxmA&ab_channel=DrazenMusa |
|
@drale1 I just double checked on the main branch is it's actually an issue there as well. The problem seems to be the implementation of the CSS variables. They seem to be set on different wrappers, and that makes the Height also adjust the Sticky Header height. Attached video for reference. Considering that is unrelated to this PR we can probably ignore it for now. I'm yet to review the most recent fixes, just wanted post an update on this issue. dxpr-glitch-2024-10-23_20.33.29.mp4 |
deuxcode
left a comment
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.
@drale1 I found 3 minor things. It's the eye-icon that shows up where it shouldn't. As these are minor I will approve this PR and pass it along to Jurriaan.
Please fix the following 3 display issues:
- There is an eye-icon for the h1 selection of the "Block Design > Title Font Size". The icon should not be there.
- There is also an eye-icon for "Block Design > Add divider to block" that souldn't be there.
- The "Sticky header > Hide before Scrolling" Always show option has an eye-icon (no-preview class on label) that should not be there.
Thanks,
jjroelofs
left a comment
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.
Ok great work guys! There is some more refactoring I would like to do but nothing serious so I'll put that in separate issues, thx for pulling through on this one!
|
I removed the .no preview class from all .form-radios and .form-checkboxes.
I spent considerable time investigating why the block_divider and block_card elements were getting the .no-preview class, but I couldn’t pinpoint the exact cause. The intended logic is that the .no-preview class should be removed in these cases. As a temporary fix, I manually removed the .no-preview class through CSS. If you’d like, I can continue searching to identify the root cause of this issue. |
Linked issues
Solution
I replaced bootstrap-slider with DXB-slider and removed all dependencies on jQuery.
I tested on Drupal 11 and it works.
But PLEASE check each function carefully because I'm not that familiar with DXPR Theme and I'm afraid something is left out.
Checklist