Skip to content

Add close button to SideBarWidget#5133

Merged
PhysSong merged 4 commits into
LMMS:masterfrom
Veratil:sidebar-close-button
Oct 1, 2019
Merged

Add close button to SideBarWidget#5133
PhysSong merged 4 commits into
LMMS:masterfrom
Veratil:sidebar-close-button

Conversation

@Veratil

@Veratil Veratil commented Aug 19, 2019

Copy link
Copy Markdown
Contributor

My take on #3682.

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

Comment thread src/gui/widgets/SideBarWidget.cpp Outdated
Comment thread src/gui/widgets/SideBarWidget.cpp Outdated
@PhysSong

Copy link
Copy Markdown
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
Comment thread src/gui/widgets/SideBar.cpp Outdated
@PhysSong PhysSong removed the needs style review A style review is currently required for this PR label Aug 26, 2019
Comment thread src/gui/widgets/SideBarWidget.cpp Outdated
@LmmsBot

LmmsBot commented Sep 29, 2019

Copy link
Copy Markdown

Downloads for this pull request

Generated by the LMMS pull requests bot.
SHA: d215412

1 similar comment
@LmmsBot

LmmsBot commented Sep 29, 2019

Copy link
Copy Markdown

Downloads for this pull request

Generated by the LMMS pull requests bot.
SHA: d215412

@PhysSong PhysSong left a comment

Copy link
Copy Markdown
Member

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
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
@rubiefawn rubiefawn removed needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels May 12, 2026
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.

5 participants