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

✨ mode selector #11

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

✨ mode selector #11

wants to merge 4 commits into from

Conversation

adrianjost
Copy link
Contributor

@adrianjost adrianjost commented Feb 28, 2024

I don't know how far I will go with this so I'm already starting a draft PR to show the progress for anyone interested to continue working on this.

It implements the basic idea of #5, making the variable modes toggelable.

Currently looks like this:

Screen.Recording.2024-02-28.at.18.23.22.mov

ToDo

  • code cleanup
  • split into more reviewable PRs not perfect but good enough I guess.
  • allow grouping by mode name, so that multiple modes with the same name can all be selected at once.
  • prettify UI good enough

@adrianjost adrianjost changed the title 🚧 WIP mode selector ✨ mode selector Feb 28, 2024
@lauthieb
Copy link
Owner

Thank you @adrianjost for beginning this!
I hope someone in the community will be able to help you.
Sorry, on my side I don't have such time in this period to work on this plugin.
Thank you again! 🙌

@adrianjost adrianjost force-pushed the feat/modes branch 3 times, most recently from 752c91c to f5cd792 Compare March 1, 2024 12:57
@adrianjost adrianjost marked this pull request as ready for review March 1, 2024 13:05
@adrianjost
Copy link
Contributor Author

adrianjost commented Mar 1, 2024

@lauthieb The code is now working as expected and ready for review. Please let me know what you think.

And sorry if the PR is not as focused as it could be. I've included a couple of small cleanups that felt like they would improve the pr readabilty.

Screen.Recording.2024-03-01.at.13.51.48.mov

@lauthieb
Copy link
Owner

lauthieb commented Mar 2, 2024

Wow thank you @adrianjost!
I'll review it soon!

@lauthieb lauthieb self-requested a review March 2, 2024 18:40
@lauthieb lauthieb added the enhancement New feature or request label Mar 2, 2024
Copy link
Owner

@lauthieb lauthieb left a comment

Choose a reason for hiding this comment

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

Thank you very much @adrianjost for your contribution!
For me it's almost ok, I just have a problem with the "All" option.

I understand that this option will add all the variables from all collections, but the problem is that the value that will be shown for one key will be the first mode.

So for example if I have a bg-primary at #FFFFFF in Light and #000000 in Dark, the "All" option will display the value of #FFFFFF for bg-primary because it's the first mode in the list, therefore "All" is not really a mode option.

Maybe instead of adding "All", it could be better to have 2 select, one for selecting the collection and the other for selecting the mode. What do you think?

Thank you again for this contrib!

- Code block copy did not work anymore
- rename All options for improved clarity, maybe I will split the collection & mode selector later.
@lauthieb
Copy link
Owner

lauthieb commented May 2, 2024

Hello @adrianjost,

Thanks for adjusting the code following my review.

I still feel uncomfortable with the idea of having only one select that mixes the concept of "All collections" and modes:
CleanShot 2024-05-03 at 00 40 52

As you can see, the plugin always show all collections. But the feature of choosing all collections or only a specific collection is a great idea! What do you think about having two select instead of one?

The first for choosing between "All collections" or a specific collection (by listing current file's variable collections)
The second for choosing the mode (by listing current modes for the current collection(s) selected).

Thank you again for this contribution! 🙌

@adrianjost
Copy link
Contributor Author

Hi @lauthieb,
I absolutely agree with your take:

Maybe instead of adding "All", it could be better to have 2 select, one for selecting the collection and the other for selecting the mode. What do you think?

I just haven't had time to implement something like that yet. My priorities at work have shifted in the last months so I couldn't commit enough time for continue working on it. I might do in the future, but I can't make promises when that would be.
But if anybody want's to continue on my work, feel free to do so.

@adrianjost
Copy link
Contributor Author

adrianjost commented May 27, 2024

@lauthieb I've made the requested change.
It is now a breaking change, because the "All variables" mode that showed the default value for all collections now does not exist anymore in this version.

But I would argue that is not necessary and the collection scoped export is the more desired behaviour. At least that what turned out most useful for us. I've initially implemented the All Collections mode because I thought we need it, but being forced to only export specific collections turned out much more useful and easier to use.

In the future, it can still be extended to multi-collection selects and showing only modes that are available in all selected collections but I personally have no use-case for it and would wait until demand from somebody is there.

Let me know what you think. 💚

Screen.Recording.2024-05-27.at.10.40.47.mov

@adrianjost adrianjost requested a review from lauthieb May 27, 2024 08:43
Fixes the Figma Warning:

> DEPRECATED: Use getVariableCollectionByIdAsync instead. This function will throw an exception if the plugin manifest contains "documentAccess": "dynamic-page".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants