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.

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

farms dialog #1684

Merged
merged 5 commits into from
Dec 14, 2023
Merged

farms dialog #1684

merged 5 commits into from
Dec 14, 2023

Conversation

amiraabouhadid
Copy link
Contributor

@amiraabouhadid amiraabouhadid commented Dec 13, 2023 •
edited
Loading

Description

move farm dialog to farms to ensure consistent closing, delete unnecessary farms dialog component

Changes

  • moved farms dialog into farms.vue
  • deleted farms dialog component

Related Issues

#1307

https://www.loom.com/share/38e5beea9efb4f0c82dc978fd9668253?sid=9a6c1f8f-f87c-4caf-b03c-f8d134df66ee

@amiraabouhadid amiraabouhadid changed the title move farm dialog to farms to ensure consisitent closing, delete unnec… farms dialog Dec 13, 2023
mohamedamer453
mohamedamer453 reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@mohamedamer453 mohamedamer453 left a comment

Choose a reason for hiding this comment

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

  • Imo the close button should be moved to the top right corner to be consistent with other dialogs in the dashboard.

    image

zaelgohary
zaelgohary reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@zaelgohary zaelgohary left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All good except for the close button. I think it should be completely removed to match the other pop-ups.

@amiraabouhadid
Copy link
Contributor Author

@zaelgohary close button was made to be consistent with nodes dialog below
image

@zaelgohary
Copy link
Contributor

Now the two headers background is a bit too much. Maybe we can reduce the toolbar's header height and change its color to match the background of the toolbar itself?

image

@zaelgohary zaelgohary merged commit 7212ffb into development Dec 14, 2023
3 checks passed
@zaelgohary zaelgohary deleted the dev_fix_dialog_close_farms branch December 14, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohamedamer453 mohamedamer453 mohamedamer453 approved these changes

@zaelgohary zaelgohary zaelgohary approved these changes

@AhmedHanafy725 AhmedHanafy725 Awaiting requested review from AhmedHanafy725 AhmedHanafy725 is a code owner

@maayarosama maayarosama Awaiting requested review from maayarosama maayarosama is a code owner

@MohamedElmdary MohamedElmdary Awaiting requested review from MohamedElmdary MohamedElmdary is a code owner

@Mahmoud-Emad Mahmoud-Emad Awaiting requested review from Mahmoud-Emad Mahmoud-Emad is a code owner

@Omarabdul3ziz Omarabdul3ziz Awaiting requested review from Omarabdul3ziz

@AlaaElattar AlaaElattar Awaiting requested review from AlaaElattar AlaaElattar is a code owner

@0oM4R 0oM4R Awaiting requested review from 0oM4R 0oM4R is a code owner

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

Successfully merging this pull request may close these issues.
3 participants
@amiraabouhadid @zaelgohary @mohamedamer453