Skip to content

Useable Mobile Editor 🎉 #2387

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

Merged
merged 45 commits into from
Sep 3, 2023
Merged

Conversation

dewanshDT
Copy link
Collaborator

@dewanshDT dewanshDT commented Aug 17, 2023

Fixes #issue-number

Changes:

The useable editor is now available in mobile!!
image

image

image

before merging this, following PRs need to be merged
#2052
#2347
#2362

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.

@dewanshDT dewanshDT requested a review from lindapaiste August 17, 2023 02:43
Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

Some first thoughts. Will continue reviewing later.


> header {
display: flex;
${prop('MobilePanel.secondary')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we’ve got a value with no property? Here and on line 21.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no this is to set the background color, it automatically does that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can also change all the foreground key in the theme to color to set the color and background automaticall by just writing this one line ${prop('some object background and color property')}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I see what you’re saying. You put the “color:” part in the theme object. So it’s like a mixin rather than a color variable.

@lindapaiste
Copy link
Collaborator

We discussed how a lot of the prevProps comparisons could be avoided if you handled each useEffect separately with the dependency on the prop you are trying to detect changes in.

@dewanshDT
Copy link
Collaborator Author

yeah I remember that, will work on all the changes now

@dewanshDT
Copy link
Collaborator Author

hey @lindapaiste, Just made all the changes and push the code check it out!

@dewanshDT dewanshDT requested a review from lindapaiste August 28, 2023 08:22
@dewanshDT dewanshDT self-assigned this Aug 28, 2023
lindapaiste and others added 3 commits September 3, 2023 15:01
* cleanup unused vars
* add dependencies to `useEffect`s
@lindapaiste lindapaiste merged commit 62df487 into processing:develop Sep 3, 2023
This was referenced Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants