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 setting navigation item blur #1503

Merged
merged 1 commit into from
Oct 21, 2020
Merged

Conversation

marcoambrosini
Copy link
Contributor

@marcoambrosini marcoambrosini commented Oct 21, 2020

Solves the first part of this comment: nextcloud/spreed#4195 (review)

Signed-off-by: Marco Ambrosini marcoambrosini@pm.me

@marcoambrosini marcoambrosini added bug Something isn't working 3. to review Waiting for reviews component Component discussion and/or suggestion labels Oct 21, 2020
@marcoambrosini marcoambrosini self-assigned this Oct 21, 2020
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Cleaner code 👍

Signed-off-by: Marco Ambrosini <marcoambrosini@pm.me>
@marcoambrosini marcoambrosini merged commit 4383295 into master Oct 21, 2020
@marcoambrosini marcoambrosini deleted the bugfix/fix-settings-blur branch October 21, 2020 14:06
Copy link
Contributor

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

There seems to be an issue with the blur logic:

  • Open the AppSettingsDialog component documentation
  • Add tens of <div>Some example content</div> to the two AppSettingsSection (so the content is too large and a vertical scroll bar is shown once the AppSettingsDialog is opened)
  • Show the settings
  • Click on the second header and then quickly click on the first header

The dialog will scroll to the second header and then to the first header. However, the first header is blurred; if instead of quickly clicking the headers you wait a little then the first header will remain highlighted after the scrolling ends.

My quick guess (not verified) by looking at the code is that after the second click is done the timer to restore linkClicked to false ends; as this happens while the scrolling is still on-going the header is blurred.

It would be nice too if, instead of blurring the headers when scrolling, the highlighted header was kept in sync with the first visible section. Although in that case maybe the headers should not be clickable if there is no scroll bar in the content... Anyway, I am just thinking out loud :-P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working component Component discussion and/or suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants