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

chore: Unreleased bugs with advanced section #4882

Merged
merged 13 commits into from
Feb 18, 2025
Merged

chore: Unreleased bugs with advanced section #4882

merged 13 commits into from
Feb 18, 2025

Conversation

kof
Copy link
Member

@kof kof commented Feb 16, 2025

Description

Steps for reproduction

  1. click button
  2. expect xyz

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • detailed review (read every line)
    • test it on preview

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 0000)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env file

@kof kof requested review from TrySound and johnsicili February 16, 2025 18:41
@kof kof marked this pull request as ready for review February 16, 2025 18:42
Copy link
Contributor

@johnsicili johnsicili left a comment

Choose a reason for hiding this comment

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

  1. Paste in/type gap: 2px, see "unset" as the value.
Screenshot 2025-02-16 at 2 29 32 PM
  1. Paste in/type gap: 2px, see you can only select shorthands (normally it converts to longhand)
Screenshot 2025-02-16 at 2 30 04 PM
  1. It still accepts invalid names
Screenshot 2025-02-16 at 2 30 58 PM

@kof
Copy link
Member Author

kof commented Feb 17, 2025

@johnsicili how exactly did you manage to get it to insert invalid property name?

@kof
Copy link
Member Author

kof commented Feb 17, 2025

Paste in/type gap: 2px, see you can only select shorthands (normally it converts to longhand)

Yeah, you either select one of the shortcuts or you escape and insert what you wrote. Should I add to the dropdown additionally the same shorthand notation user has so they understand they don't have to choose the shorthand provided?

@kof
Copy link
Member Author

kof commented Feb 17, 2025

  1. Paste in/type gap: 2px, see "unset" as the value.

Can't reproduce, how did you get that? is it possible you somehow tested the wrong branch? here is what I get

image

@johnsicili
Copy link
Contributor

how exactly did you manage to get it to insert invalid property name?

I know forsure it will add via typing/pasting full declaration displayyyyy: 2px

@johnsicili
Copy link
Contributor

Should I add to the dropdown additionally the same shorthand notation user has so they understand they don't have to choose the shorthand provided?

Yes. I did not think of hitting escape but even with knowing that, it requires an unnecessary step so adding to the autocomplete is a good idea

@johnsicili
Copy link
Contributor

Can't reproduce, how did you get that? is it possible you somehow tested the wrong branch? here is what I get

unset2.mp4

@kof
Copy link
Member Author

kof commented Feb 18, 2025

fixed, when user enters a proeprty that's unknown/wrong, it will be added but the value marked as error. Simply to avoid situation where stuff I enter disappears.

displayyyyy: 2px

image

@kof
Copy link
Member Author

kof commented Feb 18, 2025

On the second thought, to make it consistent with "entering proeprty without dashes and automatically turning it into a custom property" it seems better to have invalid properties automatically turned into custom properties always, instead of adding them with unset value

Now this: displayyyyy: 2px will turn into

image

@kof kof merged commit 7d74ea0 into main Feb 18, 2025
17 checks passed
@kof kof deleted the advanced-bugs branch February 18, 2025 12:20
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.

3 participants