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

Allow users to rename Zones #197

Merged
merged 11 commits into from
Sep 13, 2021

Conversation

BryceBeagle
Copy link
Member

@BryceBeagle BryceBeagle commented Sep 1, 2021

Resolves #194

Review along with #196 which resolves #195

TODO:

  • Don't allow a user to rename a Zone to "Screen"

Feedback on whether this is intuitive enough would be appreciated

@BryceBeagle BryceBeagle changed the title Create a StackedWidget for editable QLabels Allow users to rename Zones Sep 1, 2021
Copy link
Member

@velovix velovix left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is just me, but it looks like the name only becomes a text box if I know to click the zone name text. I think this would be very intuitive if it were just always a text box when editing the zone.

@BryceBeagle
Copy link
Member Author

That's a reasonable suggestion. Would you have the name still double-clickable when not explicitly editing the Zone?

@velovix
Copy link
Member

velovix commented Sep 7, 2021

I don't think that's necessary. Having to press the pencil to edit the zone makes sense to me.

@velovix
Copy link
Member

velovix commented Sep 8, 2021

Bryce and I decided it was chill to just have a bit of text saying to double click the zone name to change it. Not quite ideal yet, but we'll get there in the future.

@apockill
Copy link
Member

apockill commented Sep 8, 2021

Deleting a zone still causes a visual artifact:
visual-artifact

Copy link
Member

@apockill apockill left a comment

Choose a reason for hiding this comment

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

LGTM!

@BryceBeagle
Copy link
Member Author

@velovix should we make the dark theme pencil icon consistent in this PR before merge, or in another PR?

Copy link
Member

@apockill apockill left a comment

Choose a reason for hiding this comment

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

Wait no, it's buggy

@BryceBeagle
Copy link
Member Author

Issue with duplicated Alarm widgets should be resolved now

@velovix
Copy link
Member

velovix commented Sep 10, 2021

@BryceBeagle Yeah I'll give it a little bit of a fix

@velovix
Copy link
Member

velovix commented Sep 11, 2021

Okay, the "shine" of the pencil should now be transparent instead of white.

Copy link
Member

@apockill apockill left a comment

Choose a reason for hiding this comment

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

Everything seems to work!

@BryceBeagle BryceBeagle merged commit ac0ab59 into feature/195-remove-qtreewidget Sep 13, 2021
@BryceBeagle BryceBeagle deleted the feature/194-rename-zones branch September 13, 2021 17:44
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.

Stop Using QTreeWidget for ZoneList Allow users to rename Zones
3 participants