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.

Sign up for GitHub

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

Jump to bottom

Add close button to SideBarWidget #5133

Merged
merged 4 commits into from
Oct 1, 2019
Merged

Add close button to SideBarWidget #5133

merged 4 commits into from
Oct 1, 2019

Conversation

Veratil
Copy link
Contributor

@Veratil Veratil commented Aug 19, 2019

My take on #3682.

I feel like there could be a little more refactoring for finding the button/widget, but this works for now.

@PhysSong
Copy link
Member

The best way I can think of is:

  • Connect the released() signal of the close button to a lambda function
  • The lambda function hides the widget and emits the closeButtonClicked() signal
  • closeButtonClicked() is connected to corresponding SideBarButton instead of SideBar itself if possible

@Reflexe Reflexe added needs code review A functional code review is currently required for this PR needs style review A style review is currently required for this PR needs testing This pull request needs more testing labels Aug 23, 2019
@PhysSong PhysSong removed the needs style review A style review is currently required for this PR label Aug 26, 2019
@LmmsBot
Copy link

LmmsBot commented Sep 29, 2019

Downloads for this pull request

Generated by the LMMS pull requests bot.
SHA: d215412

1 similar comment
@LmmsBot
Copy link

LmmsBot commented Sep 29, 2019

Downloads for this pull request

Generated by the LMMS pull requests bot.
SHA: d215412

PhysSong
PhysSong approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

Looks good and works well.

@PhysSong PhysSong merged commit 8676399 into LMMS:master Oct 1, 2019
@PhysSong PhysSong mentioned this pull request Oct 1, 2019
Closed
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
@Veratil @PhysSong
Add close button to SideBarWidget (LMMS#5133)
463886f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhysSong PhysSong PhysSong approved these changes

Assignees

@PhysSong PhysSong

Labels
needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Veratil @PhysSong @LmmsBot @Reflexe