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

fix: [DebugBar] scroll to top #8595

Merged
merged 5 commits into from
Mar 10, 2024
Merged

Conversation

ddevsr
Copy link
Collaborator

@ddevsr ddevsr commented Mar 1, 2024

Description
Fixed #8594

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr ddevsr added the bug Verified issues on the current code behavior or pull requests that will fix them label Mar 1, 2024
@acpadhi
Copy link

acpadhi commented Mar 1, 2024

Thanks, however, this does not fix issue across the debugbar. Only fixes the toggle icon. I have uploaded 2 files in #8594 which seem to fix it for me across all links in the debugbar. Please refer to #8594 to download a zip with the changed files.

@ddevsr ddevsr changed the title fix: [DebugBar] scroll to top on open/close fix: [DebugBar] scroll to top Mar 2, 2024
@ddevsr
Copy link
Collaborator Author

ddevsr commented Mar 2, 2024

@acpadhi Updated all button

@acpadhi
Copy link

acpadhi commented Mar 2, 2024

Thanks, but Line Number : 31 - role="button" missing, so when debugbar is closed, cursor change not working when hover on toggle button.

<div id="debug-icon" class="debug-bar-ndisplay"> <a id="debug-icon-link">

There is another issue in the debugbar, when the View button is clicked, it does not highlight as selected button. Rest all buttons in the toolbar get highligted when they are toggled. Should this go as a separate issue or can be handled in this PR itself ?

@kenjis
Copy link
Member

kenjis commented Mar 4, 2024

@acpadhi

when debugbar is closed, cursor change not working when hover on toggle button.

What do you mean? What is the toggle button?

@ddevsr
Copy link
Collaborator Author

ddevsr commented Mar 4, 2024

@acpadhi Can you record?

@@ -36,6 +39,9 @@
clear: both;
text-align: center;

// Cursor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this comment, in my opinion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@acpadhi
Copy link

acpadhi commented Mar 5, 2024

@kenjis
Bottom right corner in development mode in debugbar closed state.
image
MouseOver on that icon does not change Cursor, since role=button is missing for that anchor tag.

@acpadhi
Copy link

acpadhi commented Mar 5, 2024

@ddevsr
Since you are working on the CSS/SCSS, please look into this as well :

image

In debugbar when someone clicks on the Views button to show the views higlighted in the page, the button in the toolbar should also have a darker background to highlight that the views button has been toggled to an ON state, just like how the other buttons in the debugbar behave. If this is confusing, please let me know and I will write back again to explain.

@kenjis
Copy link
Member

kenjis commented Mar 5, 2024

@acpadhi

2024-03-05.14.21.59.mov

@kenjis
Copy link
Member

kenjis commented Mar 5, 2024

In debugbar when someone clicks on the Views button to show the views higlighted in the page, the button in the toolbar should also have a darker background to highlight that the views button has been toggled to an ON state, just like how the other buttons in the debugbar behave.

"Views" tab shows nothing, so isn't the behavior intentional?
At least, the behavior is the same in v4.0.1.

Clicked "Files":
Screenshot 2024-03-05 14 38 17

and clicked "Views":
Screenshot 2024-03-05 14 38 27

@acpadhi
Copy link

acpadhi commented Mar 5, 2024

@kenjis

"Views" tab shows nothing, so isn't the behavior intentional?
At least, the behavior is the same in v4.0.1.

In my Opinion, it is kind of not directly imminent that that the view button is clicked, I understant that there is border highlithing on page view areas, but when working on a large pages, visually it can be distracting to see gaps in design and wondering if some css is causing it. A dark background in the button will immediately convey "Oh my debugbar view button is toggled, so I see the gaps" . This is not a pressing thing, just a suggestion to keep consistency, since afterall it is a toggle, irrespective of whenther showing additional content or not.

@kenjis
Copy link
Member

kenjis commented Mar 5, 2024

@acpadhi Ah, I got what you say. You say it is better that when the "Views" is on, the "Views" is darker.
It would certainly be better that way.

@acpadhi
Copy link

acpadhi commented Mar 5, 2024

Ah, I got what you say. You say it is better that when the "Views" is on, the "Views" is darker.
It would certainly be better that way.

Yes, that way this Views button will be conistent with other buttons on the visual aspect, and also make it exlicit that it is in ON state.

@kenjis
Copy link
Member

kenjis commented Mar 5, 2024

@acpadhi I agree with your opinion. But the current behavior is not a bug. So changing it will be an enhancement.

This PR seems good to me.

kenjis
kenjis previously approved these changes Mar 5, 2024
@ddevsr
Copy link
Collaborator Author

ddevsr commented Mar 6, 2024

@acpadhi Ah, i see. If view button already click, marking is ACTIVE right?

@kenjis Next PR?

@kenjis
Copy link
Member

kenjis commented Mar 6, 2024

@ddevsr It is an enhancement, not a bug fix. Please send a new PR to 4.5 if you send.

@kenjis kenjis dismissed their stale review March 6, 2024 07:52

Needs more consideration

@ddevsr ddevsr force-pushed the debug-bar-scroll-top branch from 99bdf23 to 0fc5485 Compare March 8, 2024 23:46
@ddevsr ddevsr requested review from kenjis and paulbalandan March 9, 2024 00:38
@kenjis
Copy link
Member

kenjis commented Mar 9, 2024

The following error will be fixed by #8612.

 Error: Ignored error pattern #^Cannot unset offset string on array\<int, int\>\|null\.$# in path /home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Mock/MockCache.php was not matched in reported errors.
 ------ ---------------------------------------------------------------------------- 
  Line   system/Test/Mock/MockCache.php                                              
 ------ ---------------------------------------------------------------------------- 
         Ignored error pattern #^Cannot unset offset string on array<int,            
         int>\|null\.$# in path                                                      
         /home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/Mock/MockCache.php  
         was not matched in reported errors.                                         
 ------ ---------------------------------------------------------------------------- 

https://github.com/codeigniter4/CodeIgniter4/actions/runs/8210351538/job/22457631537?pr=8595

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kenjis kenjis merged commit e6052cf into codeigniter4:develop Mar 10, 2024
62 of 63 checks passed
@ddevsr ddevsr deleted the debug-bar-scroll-top branch March 10, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Debugbar Click(toggle) Scrolls Document to Top
4 participants