Skip to content

Convert Toolbar to a function component #2352

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Aug 13, 2023
Merged

Convert Toolbar to a function component #2352

merged 7 commits into from
Aug 13, 2023

Conversation

dewanshDT
Copy link
Collaborator

@dewanshDT dewanshDT commented Aug 5, 2023 •
edited
Loading

Changes:
Rewrote the old class based Toolbar to a functional component, along with new test file

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 5, 2023 22:07
lindapaiste
lindapaiste suggested changes Aug 5, 2023
View reviewed changes
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.

Looks mostly good. I need to load up the branch and make sure that everything works.

client/modules/IDE/components/Header/Toolbar.jsx
}
function handleProjectNameSave() {
const newName = nameInputValue;
if (newName.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The handling of 0 length is a bit weird in the current version. If you click outside the input it reverts the text but keeps the input open. It seems like yours does nothing which is probably fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let me know If you want me to change this

@lindapaiste lindapaiste changed the title New Functional Toolbar component Convert Toolbar to a function component Aug 5, 2023
@dewanshDT
Copy link
Collaborator Author

Hey @lindapaiste! I just updated this with all the suggested changes, please check it out

@dewanshDT dewanshDT requested a review from lindapaiste August 6, 2023 18:03
@lindapaiste lindapaiste mentioned this pull request Aug 7, 2023
Closed
24 tasks
@lindapaiste
Copy link
Collaborator

@dewanshDT I'm really not sure what our plan is with the toolbar. Like what changes we intend to make and in what order.
Should I merge this and then resolve conflicts to apply #2238 on top of this?

@dewanshDT
Copy link
Collaborator Author

yes I think we should merge this and then resolve the conflicts to apply #2238, and then we can discuss the future changes than we want to make

@lindapaiste lindapaiste merged commit e668ae4 into processing:develop Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lindapaiste lindapaiste lindapaiste approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dewanshDT @lindapaiste