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

Add setting option to exclude mode from token value #309

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

Conversation

0m4r
Copy link
Collaborator

@0m4r 0m4r commented Jul 5, 2024

  • new setting to allow to only preserve mode in token name but exclude it from token value
  • add documentation in README.md
  • extend Checkbox component to embed Info component inline to the checkbox label
  • update jest moduleMapper to resolve "@src"

Fix #308

* new setting to allow to only preserve mode in token name but exclude it from token value
* add documentation in README.md
* extend Checkbox component to ember Info component inline to the checkbox label
* update jest moduleMapper to resolve "@src"
@0m4r
Copy link
Collaborator Author

0m4r commented Jul 18, 2024

@lukasoppermann hi!
I am wondering if I am supposed to take any further step to have this PR approved or declined.
I am not familiar with the process, thus I am asking 😀

@lukasoppermann
Copy link
Owner

Hey @0m4r,

this is good, I just have to review it.

I will probably remove the contributors section.

I am happy for it to be added, but then we need to do a new PR and add all people who contributed.

@lukasoppermann
Copy link
Owner

Hey @0m4r I added a PR here: 0m4r#1

But it is still, not working. The name is not shown in the JSON structure.

@0m4r
Copy link
Collaborator Author

0m4r commented Jul 19, 2024

(I am not close to a computer to test at the moment)

But, could you help me to replicate your settings with a screenshot?

The "new" behavior should be setup by ticking off the "old" checkbox, and tick on the "new" one.

...now that I think about it may not be the best user experience, tho 😓

@lukasoppermann
Copy link
Owner

Hey, I updated the naming a bit but this does not matter.

However, no matter if the new setting is on or off, it never adds the mode to the name.

Adding to the name for me means it should be in the json structure

{
  base: { // <- (collection)
    light: { // <- (mode)
      green: {...}

This is also how the name is referenced in the value (if the new setting is on). But this does not happen.

I expect (but could not test it) that we are missing something in the export or somewhere. I also did not log the name to see if it is actually added.

Just FYI I am on vacation for two weeks very soon, so I may not answer for two weeks.

@0m4r
Copy link
Collaborator Author

0m4r commented Jul 22, 2024

I have run a few tests and reported the findings here:
0m4r#1 (comment)
what do you think?

@lukasoppermann
Copy link
Owner

Hey @0m4r are you still on this? I agree that the UX seems a bit confusing. Any idea how to fix it?

@0m4r
Copy link
Collaborator Author

0m4r commented Sep 19, 2024

Hi Lukas, I am still interested to fix this, but in the last weeks I have not really got a lot of time to spend on this.

Let me come back to you with some ideas...
(Especially about the issue of referencing variable in the same mode - that seems to be the weirdest issue so far)

Add setting configuration to add the mode name to the token name or value.
Both token name and value can have the mode added, only one of them or none.
@0m4r
Copy link
Collaborator Author

0m4r commented Sep 26, 2024

hi @lukasoppermann ,
I came up with a solution that seems to work for the most common use cases.
There are 2 options now:

  • use the mode name in the token name
  • use the mode name in the token value

by combining them, name and value can both be generated including the mode name, only one of them or none.

image

The problem with values that reference variables in the same mode remains unresolved.
I am not able to figure out now if and how this information is available to be detected.

(I have not updated screenshots and docs yet - I wanted to have a review of the current idea before completing all the work )

@0m4r 0m4r marked this pull request as draft September 26, 2024 20:18
@0m4r 0m4r self-assigned this Sep 27, 2024
@0m4r 0m4r added the bug Something isn't working label Sep 27, 2024
@coveralls
Copy link

coveralls commented Sep 30, 2024

Pull Request Test Coverage Report for Build 11109230927

Details

  • 8 of 27 (29.63%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.9%) to 71.04%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utilities/getVariables.ts 6 25 24.0%
Totals Coverage Status
Change from base Build 10947804742: 1.9%
Covered Lines: 584
Relevant Lines: 807

💛 - Coveralls

@0m4r
Copy link
Collaborator Author

0m4r commented Sep 30, 2024

@lukasoppermann I have now completed the work on this PR and updated the README.md.
It is now also resolving the alias within the same collection/mode.

The code is not great, I know... still, what do you think?

@0m4r 0m4r marked this pull request as ready for review September 30, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mode name are duplicated in referenced values
4 participants