Skip to content
This repository has been archived by the owner on Sep 25, 2024. It is now read-only.

[Mac] Initial Variations implementation #600

Closed
wants to merge 2 commits into from

Conversation

CartBlanche
Copy link
Contributor

No description provided.

@CartBlanche CartBlanche self-assigned this May 16, 2019
@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 2 times, most recently from c0bfc0c to cb6103b Compare May 24, 2019 03:53
@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 2 times, most recently from 21b47a7 to 1f01ec4 Compare June 12, 2019 16:24
@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 5 times, most recently from 7bc6786 to 774fc34 Compare June 26, 2019 18:39
@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 4 times, most recently from 5fdabb4 to 1f5e720 Compare July 3, 2019 09:30
@CartBlanche
Copy link
Contributor Author

Current L&F of
Variations Dialog:
Screen Shot 2019-07-03 at 10 36 10

Variations Tree Rendering:
Screen Shot 2019-07-03 at 10 35 20

@CartBlanche
Copy link
Contributor Author

CartBlanche commented Jul 3, 2019

@vancura Could you please cast your eye over the screenshots above ^^

@vancura
Copy link
Contributor

vancura commented Jul 3, 2019

Hey,

  • There's a huge space between dropdowns and buttons on the top image
  • Wrong font size in the variation label (1pt lower, please)
  • Please don't use "X" and "+" characters on add/remove. We have bitmaps for these.

@ermau
Copy link
Member

ermau commented Jul 3, 2019

@vancura I'm curious about the change from the popup to a dialog, what was the motivation?

@CartBlanche
Copy link
Contributor Author

CartBlanche commented Jul 3, 2019

@vancura I don't have the bitmaps you speak of.

I have an add and remove image, but when used look like this, not like the figma...

Screen Shot 2019-07-03 at 17 31 02

If you have them, please send them to me.

@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 2 times, most recently from 6bcb9d2 to a892d4e Compare July 4, 2019 15:54
@CartBlanche
Copy link
Contributor Author

CartBlanche commented Jul 4, 2019

@vancura
Screen Shot 2019-07-04 at 18 16 10

Font - 1
Screen Shot 2019-07-03 at 17 37 28

Also please see my comment above about the missing images.

@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 3 times, most recently from da3d76a to f40f1da Compare July 11, 2019 18:28
@CartBlanche CartBlanche requested a review from ermau July 11, 2019 18:33
@CartBlanche CartBlanche marked this pull request as ready for review July 11, 2019 18:34
Copy link
Member

@ermau ermau left a comment

Choose a reason for hiding this comment

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

Bugs:

  • Changing theme after having the add variation dialog creates two dialogs the next time you invoke it.
  • Switch to dark mode, add a variant, unselect the mock and reselect it. The line is the wrong color.
  • Plus seems to turn a wrong color when the dialog is open.
  • Add a variant, deselect mock and reselect. Variant is now readonly?
  • Add a variant in grouped mode, variant is readonly.

@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 2 times, most recently from 1f9c340 to 1049a57 Compare September 30, 2019 17:02
@ermau
Copy link
Member

ermau commented Sep 30, 2019

I think we might want to add another property that the auto-focus on property button press thing uses to focus. Default it to the first key view and make it virtual. For variant properties, the remove variant button is the thing that gets focused after pressing the property button and that doesn't seem like the ideal behavior.

@CartBlanche CartBlanche force-pushed the dominique-MacVariations branch 3 times, most recently from 3b6b044 to 17be6ce Compare October 7, 2019 12:10
@ermau ermau removed their assignment Oct 7, 2019
@CartBlanche
Copy link
Contributor Author

Due to time, I've added a separate issue with regard to the autofocus property button - #652

@ermau
Copy link
Member

ermau commented Oct 9, 2019

Due to time? This doesn't have a desirable behavior, it is not a separate issue.

@CartBlanche
Copy link
Contributor Author

CartBlanche commented Oct 10, 2019

By Due to time, I'm talking about what Bill said. Also that desirable behaviour would be required if Variations was shipping, which at the moment, my understanding is that it is on hold. Hence why I set up the a separate issue. So that if it if we need to work on it again, there is already an issue for it. Also it isn't that the current behaviour is unusable. It makes sense when tabbing. The other scenario is what needs enhancing.

@ermau
Copy link
Member

ermau commented Oct 10, 2019

Variations are also intended for Form's use, they are outside of the scope of the discussion with Bill.

@@ -154,8 +164,8 @@ private void PopUpContextMenu ()

private void FocusClickedRow ()
{
if (Superview != null) {
MakeFocusableKeyViewFirstResponder (Superview.Subviews);
if (Superview is EditorContainer ec) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By only passing the EditorView.NativeView.Subviews the recursion only traverses down the EditorContainer tree, thus avoiding the Variation buttons.

@ermau ermau removed their assignment May 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants