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

Fixed scrollbar disappearing after changing content #2303

Merged
merged 1 commit into from Jul 28, 2021

Conversation

s77rt
Copy link
Contributor

@s77rt s77rt commented Jun 29, 2021

Description:

If a scrollbar content is changed, the whole widget content will be replaced (including the scrolling bars, shadows) with just the new content.
The fix is to change only the content of the scrollbar (keeping other objects untouched: scrollbars, shadows, etc)

Fixes #2268

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@s77rt
Copy link
Contributor Author

s77rt commented Jun 29, 2021

DocTabs didn't have any scrollbars, making this change will fix the mentioned issue, but will also make DocTabs able to have scrollbars
I like how DocTabs look without scrollbars however with scrollbars it does not look as good (having two bars, bottomline of DocTabs and the scrollbar)
Screenshot from 2021-06-29 03-53-51

any ideas?

@andydotxyz
Copy link
Member

If content is scrollable it should have scroll bars I think. Otherwise how do people see that the content can scroll. I don’t know if this is an omission in DocTabs, perhaps @stuartmscott can recall?

@andydotxyz
Copy link
Member

The test failures need to be addressed before this can be considered. You’ll see the same issues using go test locally. You’ll need to copy files from testdata/failed over to testdata/ if the change is intended. It then appears as part of the PR change set.

@s77rt
Copy link
Contributor Author

s77rt commented Jul 5, 2021

okay, but what about the design?
it does not look as good as it was, having two bars is not a good thing IMHO

@andydotxyz
Copy link
Member

it does not look as good as it was, having two bars is not a good thing IMHO

I go with the idea that scrollbars should be used to indicate scrollable content. I don't think we should remove the shadow line as it will not always be scrollable.
Perhaps we will be forced into adding a "Hide Scrollbars" feature?

@s77rt
Copy link
Contributor Author

s77rt commented Jul 6, 2021

or maybe the scrollbar must have it's own dedicated space, not an overlay

@zdima
Copy link
Contributor

zdima commented Jul 7, 2021

I agree the scroll bar doesn't look good with DocTabs, and perhaps we should disable the scroll bar in DocTabs.
However, the DocTabs implementation seems incomplete. The AppTabs and DocTabs are very much alike. But you will see the difference in how the bottom line is handled. In the AppTabs, when "invisible" tab selected, the bottom line moved under "...". The DocTabs bottom line just disappeared.
Also for both DocTabs and AppTabs, when "invisible" tab selected and user open popup with "...", there is no indication which tab is currently actually.

@andydotxyz
Copy link
Member

AppTabs and DocTabs are indeed different. The app tabbing is designed for a more static layout (the main areas of an app at the beginning of a list, others accessible through popup if screen space is too small).
The doc tabs however are designed for lots of documents that may be opened or closed. When you choose an item from the doctabs menu it should scroll in to view.

@Jacalz
Copy link
Member

Jacalz commented Jul 26, 2021

I had a look at this and I think DocTabs certainly look better without the scrollbar. If it isn't too much of a hassle to turn it off for that use case, I think it would be useful to implement that. Is there any broader use case for having that option available in a public API?

@andydotxyz
Copy link
Member

If we want this to land in 2.0.4 then we should ignore DocTabs for now, as it does not exist until 2.1... @Jacalz?

@Jacalz
Copy link
Member

Jacalz commented Jul 27, 2021

That is a very good point. We can tackle that separately.

@andydotxyz
Copy link
Member

Hey @s77rt we are looking to prep v2.0.4 this week - do you think we could wrap this up (fixing tests etc) and we can review DocTabs as we move to 2.1 after.

@s77rt
Copy link
Contributor Author

s77rt commented Jul 27, 2021

Hi,
should i just copy the new results as the expected results

You’ll need to copy files from testdata/failed over to testdata/ if the change is intended.

@andydotxyz
Copy link
Member

should i just copy the new results as the expected results

Yes. When you push that into the PR we can see the change in expected results to confirm what has altered.

@s77rt
Copy link
Contributor Author

s77rt commented Jul 27, 2021

hmm what about mobile tests?

@Jacalz
Copy link
Member

Jacalz commented Jul 27, 2021

You have to run the tests with go test -tags mobile . to get those as well.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Great update thanks. However it looks like one or two of the tests have additional changes.

@andydotxyz
Copy link
Member

Apologies some other features have landed so you'll need to merge develop and re-run the tests.
Perhaps the unexpected change goes away then, I'm not sure what caused it.

@s77rt s77rt marked this pull request as draft July 28, 2021 15:40
@s77rt
Copy link
Contributor Author

s77rt commented Jul 28, 2021

this change also produces a new test failure:
TestWindow_HandleOutsideHoverableObject : (testdata has not been replaced for this one) i think this is something just from my side

@s77rt s77rt marked this pull request as ready for review July 28, 2021 16:05
@andydotxyz
Copy link
Member

I have started to see that test fail as well, so I don't think its your fault.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Excellent thanks

@andydotxyz
Copy link
Member

Are you happy too @Jacalz ?

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Yep :)

@andydotxyz andydotxyz merged commit 2ce1063 into fyne-io:develop Jul 28, 2021
@andydotxyz
Copy link
Member

FYI for future PRs please don't force push after a review is started as it can make it harder to do subsequent review passes.

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.

None yet

4 participants