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 group height in overview mode #366

Merged
merged 3 commits into from
May 14, 2020

Conversation

thinkh
Copy link
Member

@thinkh thinkh commented May 13, 2020

Fixes #365

Summary

Uses the same group height in overview mode.

ordino-overview-group-height

Implementation

Previously, the group height is set as hard-coded values and ignores the LineUp/Taggle default config values.

This refactoring uses the taggle options (LineUp default config + ARankingView custom options).

The event LineUpPanelActions.EVENT_RULE_CHANGED was removed and replaced with the LineUpPanelActions.EVENT_TOGGLE_OVERVIEW which simply returns a boolean. Afterwards the space-filling rule is only instanciated and used in ARankingView.

You can change the heights by defining the respective LineUp/Taggle config values in the customOptions of ARankingView.

const options = {
    // ...
    customOptions: {
        rowHeight: 18,
        groupHeight: 70,
        groupPadding: 5
    }
}

thinkh added 2 commits May 13, 2020 11:14
Closes #365

Previously, the group height is set as hard-coded values and ignores the LineUp/Taggle default config values.

This refactoring uses the taggle options (LineUp default config + ARankingView custom options).

The event `LineUpPanelActions.EVENT_RULE_CHANGED` was removed and replaced with the `LineUpPanelActions.EVENT_TOGGLE_OVERVIEW` which simply returns a boolean. Afterwards the space-filling rule is only instanciated and used in ARankingView.
@thinkh thinkh added type: bug Something isn't working type: refactor Refactor the current implementation release: patch PR merge results in a new patch version labels May 13, 2020
@thinkh thinkh requested a review from oltionchampari May 13, 2020 09:25
@thinkh thinkh changed the title Thinkh/365 fix overview group height Fix group height in overview mode May 13, 2020
@thinkh thinkh marked this pull request as ready for review May 13, 2020 11:05
@thinkh thinkh requested a review from lehnerchristian as a code owner May 13, 2020 11:05
Copy link
Contributor

@oltionchampari oltionchampari left a comment

Choose a reason for hiding this comment

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

This fix works for me as expected and the source code looks good. 👍

This fix made another issue more obvious though.
The bars of the groups appear and disappear when switching back and forth to overview mode.

groups_bug

This should be addressed in another issue though since it occurs also for the lineupjs4 branch. It is harder to spot because of the bars changing their height.

@thinkh If you agree to address this in a new issue we can merge this branch.

@thinkh
Copy link
Member Author

thinkh commented May 14, 2020

Thanks for testing and well spotted! Can you please extract the bug you've found to a new issue? We can adress it in a different PR.

You can go ahead and merge this PR.

@oltionchampari
Copy link
Contributor

Thanks for testing and well spotted! Can you please extract the bug you've found to a new issue? We can adress it in a different PR.

You can go ahead and merge this PR.

Okay, will do.

@oltionchampari oltionchampari merged commit 34d9d26 into lineupjs4 May 14, 2020
@oltionchampari oltionchampari deleted the thinkh/365_fix-overview-group-height branch May 14, 2020 09:07
@thinkh thinkh mentioned this pull request Jun 15, 2020
@thinkh thinkh mentioned this pull request Jul 30, 2020
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: patch PR merge results in a new patch version type: bug Something isn't working type: refactor Refactor the current implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants