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

Create new instance btn and go to next prompt #208

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

juayuohcarineneng19
Copy link
Contributor

@juayuohcarineneng19 juayuohcarineneng19 commented Jun 25, 2024

The design outlined below addresses the navigation (Create new instance button and the go to next prompt button) issues in the ODK-Survey form .

The colors, font size, type, and spacing used adhere to the ODK-X Design guidelines

Any feedback or additional information would be greatly appreciated. Thank you.

On the first grid, image one compares the old instance button to the new one.

newinstance

On the second grid, image one compares the go to next prompt button to the new one
go to next prompt

@juayuohcarineneng19 juayuohcarineneng19 changed the base branch from main to development June 25, 2024 12:16
@Redeem-Grimm-Satoshi
Copy link
Member

Redeem-Grimm-Satoshi commented Jun 26, 2024

@juayuohcarineneng19 Great work!

  1. Can you provide a better screenshot like before and after the changes were made? side by side comparison would be better. It'll make it easier to review your PR by letting us know what was there before and what wasn't there without having to run the app

  2. This PR title "Create new instance btn" seems vague, you did modifications not only on the Create new instance btn. It's important

*Code will be reviewed after this minor changes

@juayuohcarineneng19 juayuohcarineneng19 changed the title Create new instance btn Create new instance btn and go to next prompt Jun 27, 2024
@juayuohcarineneng19
Copy link
Contributor Author

Thank you . PR updated @Redeem-Grimm-Satoshi

@wbrunette
Copy link
Member

@juayuohcarineneng19 I noticed you are adding grunt-shell to our environment is there a reason for that? why was it necessary? we try to minimize the number of packages needed for app-designer since users often have limited connectivity. Is there something that that grunt-cli could not do that you needed? @r0ssing

@juayuohcarineneng19
Copy link
Contributor Author

@juayuohcarineneng19 I noticed you are adding grunt-shell to our environment is there a reason for that? why was it necessary? we try to minimize the number of packages needed for app-designer since users often have limited connectivity. Is there something that that grunt-cli could not do that you needed? @r0ssing

Please I did not notice I was adding the grunt-shell to our environment. I will remove this. Thank You

@maprehensive
Copy link
Contributor

@juayuohcarineneng19 Sorry for the delay reviewing.

Button is in line with the style guide, and it looks good. I'm ok with the changes.

@maprehensive
Copy link
Contributor

@Redeem-Grimm-Satoshi I have reviewed design - can you review code and merge if ok?

Copy link
Contributor

@maprehensive maprehensive left a comment

Choose a reason for hiding this comment

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

Design looks good to me - happy to approve pending Waylon's comments about gruntfile and Redeem's code review.

@Redeem-Grimm-Satoshi
Copy link
Member

@Redeem-Grimm-Satoshi I have reviewed design - can you review code and merge if ok?

@maprehensive Yes sure

@Redeem-Grimm-Satoshi
Copy link
Member

Looks good!

@Redeem-Grimm-Satoshi
Copy link
Member

Redeem-Grimm-Satoshi commented Aug 13, 2024

@Chinex-Boroja Please check this out for any testing related issues
@r0ssing This issue is ready to be merged :)

@wbrunette wbrunette requested a review from r0ssing August 13, 2024 15:42
Copy link
Member

@Redeem-Grimm-Satoshi Redeem-Grimm-Satoshi left a comment

Choose a reason for hiding this comment

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

I just noticed this branch have some conflicts, @juayuohcarineneng19 try resolving them first

@juayuohcarineneng19
Copy link
Contributor Author

I just noticed this branch have some conflicts, @juayuohcarineneng19 try resolving them first

Conflicts resolved @Redeem-Grimm-Satoshi

@Chinex-Boroja
Copy link

@Chinex-Boroja Please check this out for any testing related issues @r0ssing This issue is ready to be merged :)

Hi @Redeem-Grimm-Satoshi, I am not really sure what I am meant to do here or how to go about this regarding writing tests, if that is what you mean. If you could explain, it will be quite helpful. Thank you

@juayuohcarineneng19
Copy link
Contributor Author

juayuohcarineneng19 commented Aug 15, 2024

@Chinex-Boroja Please check this out for any testing related issues

@r0ssing This issue is ready to be merged :)

Hi @Redeem-Grimm-Satoshi , please it was mentioned in our meeting that @Chinex-Boroja can only write test for tables app and not app designer as he isn't familiar with app designer .

@Redeem-Grimm-Satoshi
Copy link
Member

@r0ssing Please go ahead and merge this, looks good

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.

5 participants