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

Reduce hit target of more memory loader #143

Conversation

colin-grant-work
Copy link
Contributor

What it does

Fixes #134

The previous code didn't set the header wrapping div to be flex, and it didn't justify content itself, meaning that the 'more memory loader' took up the full width.

How to test

  1. Confirm that the hit target of the more memory loader is limited to the text itself.

Alternatively, the more memory renderer could wrap itself so that it still took up the whole width, but only the text was the hit target. If this seems preferable, it can be implemented.

Review checklist

Reminder for reviewers

@jreineckearm
Copy link
Contributor

Thanks a lot, @colin-grant-work , for picking this up! @arekzaluski , could you please help with reviewing this one?

Copy link
Contributor

@arekzaluski arekzaluski left a comment

Choose a reason for hiding this comment

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

LGTM. Issue is fixed. While testing it I spotted two other small issues. They however also exists on main so unrelated to this change.

  1. There is no pointer cursor when hovering over the dropdown for selecting the number of bytes to load
  2. Clicking the dropdown to select the ammount of bytes is automatically fetching more data. That should not be the case as user only selects the ammount of bytes to load at that point

@colin-grant-work
Copy link
Contributor Author

@arekzaluski, good catches. I've made a new issue (#144) for those.

@colin-grant-work colin-grant-work merged commit d9c6296 into eclipse-cdt-cloud:main Jul 9, 2024
5 checks passed
@colin-grant-work colin-grant-work deleted the bugfix/smaller-more-memory-target branch July 9, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Load <n> more bytes above/below" button should be only sensitive to mouse hovering over the text
3 participants