Skip to content

fix: [DebugBar] scroll to top - #8595

Merged
kenjis merged 5 commits into
codeigniter4:developfrom
ddevsr:debug-bar-scroll-top
Mar 10, 2024
Merged

kenjis merged 5 commits into
codeigniter4:developfrom
ddevsr:debug-bar-scroll-top

Conversation

@ddevsr

@ddevsr ddevsr commented Mar 1, 2024

Copy link
Copy Markdown
Collaborator

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

acpadhi commented Mar 1, 2024

Copy link
Copy Markdown

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

ddevsr commented Mar 2, 2024

Copy link
Copy Markdown
Collaborator Author

@acpadhi Updated all button

@acpadhi

acpadhi commented Mar 2, 2024

Copy link
Copy Markdown

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

kenjis commented Mar 4, 2024

Copy link
Copy Markdown
Member

@acpadhi

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

What do you mean? What is the toggle button?

@ddevsr

ddevsr commented Mar 4, 2024

Copy link
Copy Markdown
Collaborator Author

@acpadhi Can you record?

Comment thread admin/css/debug-toolbar/toolbar.scss Outdated
clear: both;
text-align: center;

// Cursor

Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

@acpadhi

acpadhi commented Mar 5, 2024

Copy link
Copy Markdown

@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

acpadhi commented Mar 5, 2024

Copy link
Copy Markdown

@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

kenjis commented Mar 5, 2024

Copy link
Copy Markdown
Member

@acpadhi

2024-03-05.14.21.59.mov

@kenjis

kenjis commented Mar 5, 2024

Copy link
Copy Markdown
Member

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

acpadhi commented Mar 5, 2024

Copy link
Copy Markdown

@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

kenjis commented Mar 5, 2024

Copy link
Copy Markdown
Member

@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

acpadhi commented Mar 5, 2024

Copy link
Copy Markdown

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

kenjis commented Mar 5, 2024

Copy link
Copy Markdown
Member

@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

ddevsr commented Mar 6, 2024

Copy link
Copy Markdown
Collaborator Author

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

@kenjis Next PR?

@kenjis

kenjis commented Mar 6, 2024

Copy link
Copy Markdown
Member

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

Comment thread admin/css/debug-toolbar/toolbar.scss Outdated
@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

kenjis commented Mar 9, 2024

Copy link
Copy Markdown
Member

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

@kenjis kenjis left a comment

Copy link
Copy Markdown
Member

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
@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