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

Vertical Scrollable List Fragment #373

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

nanodeath
Copy link
Contributor

This is a possible implementation for the Vertical Scrollable List thing in #372.

I've refactored a lot of the previous work found in ScrollBarExample.kt into a reusable Fragment. I also implemented some bug fixes in the scrollbar implementation itself, but I tried to keep these minimal to not pollute the PR, but for example in tiny scroll bars (height 2 for example) the bar doesn't render at all, when it should render with height 1.

I've included a shiny new vaguely-named TestRenderer that lets me render the entire component stack and trigger events. It shouldn't really be in this PR, as it's not related to the new component, but it's at least a separate commit.

I've also updated ScrollBarExample to use the new fragment.

Happy to incorporate suggestions.

Open Questions

  • There are a lot of constructor arguments for this fragment. Should we start a Fragments.newVerticalScrollableList() pattern going, to complement Components?
  • Are the Limitations I listed in the KDoc for VerticalScrollableList acceptable, or are they indicative of a fundamental problem?
  • I marked the new ScrollableList interface as @Beta, is that fine?

Like PR #371, this change is stacked on top of #369 because of the helpers I introduced there, so I'm marking this as a draft.

Closes #372.

@nanodeath nanodeath force-pushed the scrollable-controls branch 3 times, most recently from ff44c78 to 32af8e9 Compare April 24, 2021 04:56
@nanodeath nanodeath force-pushed the scrollable-controls branch from 32af8e9 to d992f8a Compare April 24, 2021 05:00
@nanodeath nanodeath marked this pull request as ready for review April 24, 2021 05:02
@nanodeath
Copy link
Contributor Author

Ready for review, assuming the CircleCI build succeeds. Open questions are still open though!

Copy link
Member

@adam-arold adam-arold left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -168,6 +167,20 @@ abstract class BaseScrollBar(
} else Pass
}

override fun mouseWheelRotatedUp(event: MouseEvent, phase: UIEventPhase): UIEventResponse {
Copy link
Member

Choose a reason for hiding this comment

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

This is really cool!

@@ -8,10 +8,12 @@ import org.hexworks.zircon.api.data.Position
import org.hexworks.zircon.api.data.Tile
import org.hexworks.zircon.api.graphics.TileGraphics

@Suppress("DuplicatedCode")
class VerticalScrollBarRenderer : ComponentRenderer<ScrollBar> {
open class VerticalScrollBarRenderer internal constructor() : ComponentRenderer<ScrollBar> {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you make it open?

@adam-arold adam-arold merged commit adcda81 into Hexworks:master Apr 27, 2021
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.

Vertical Scrollable List Component
2 participants