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

ODK-X Instance row control buttons. #211

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

Conversation

juayuohcarineneng19
Copy link
Contributor

@juayuohcarineneng19 juayuohcarineneng19 commented Jul 8, 2024

ODK-X Instance row control buttons adjusted to fit on one row (edit and delete icons).

ODK-X Survey: Update the buttons, icons and list view of the home screen

WhatsApp Image 2024-07-08 at 3 04 00 PM

@juayuohcarineneng19 juayuohcarineneng19 changed the base branch from main to development July 8, 2024 13:00
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.

Hi @juayuohcarineneng19 , nice work.

One suggestion I have is to add some padding between these two buttons to reduce the likelihood of tapping the wrong button.

This layout looks better visually, but you could address the potential usability problem as well.

@juayuohcarineneng19
Copy link
Contributor Author

juayuohcarineneng19 commented Jul 9, 2024 via email

@juayuohcarineneng19
Copy link
Contributor Author

Added a gap of 5px.

WhatsApp Image 2024-07-09 at 4 08 59 PM

@nuilewis
Copy link

Permit me to nitpick please😅
If it's possible to be adjusted, then great... if not then don't waste too much time on it, you can move on to other things.

Design

@Redeem-Grimm-Satoshi
Copy link
Member

Redeem-Grimm-Satoshi commented Jul 11, 2024

Great but I haven't seen a change in the icon ( edit, delete ), the icons had to be changed to the material ones. Perhaps, the delete icon doesn't serve it purpose, it should be changed to the trash icon ( you know what I mean )

@nuilewis Great suggestion there, you just asked what I had in mind

@Redeem-Grimm-Satoshi
Copy link
Member

@Redeem-Grimm-Satoshi please this pull request is linked to this design. So I was mainly concern about the design.

Here is the issue on updating every icon to material design guideline

I look forward to working on all of these issues. thank you.

@juayuohcarineneng19 Isn't the icons part of the design? I want the icon changes included in this PR

@juayuohcarineneng19
Copy link
Contributor Author

@Redeem-Grimm-Satoshi please this pull request is linked to this design. So I was mainly concern about the design.
Here is the issue on updating every icon to material design guideline
I look forward to working on all of these issues. thank you.

@juayuohcarineneng19 Isn't the icons part of the design? I want the icon changes included in this PR

Sure @Redeem-Grimm-Satoshi working on it now.

@juayuohcarineneng19
Copy link
Contributor Author

Permit me to nitpick please😅 If it's possible to be adjusted, then great... if not then don't waste too much time on it, you can move on to other things.

Design

Thank you @nuilewis . Taking these into consideration.

@juayuohcarineneng19
Copy link
Contributor Author

Permit me to nitpick please😅 If it's possible to be adjusted, then great... if not then don't waste too much time on it, you can move on to other things.
Design

Thank you @nuilewis . Taking these into consideration.

@nuilewis @Redeem-Grimm-Satoshi here is what I have now. I am still learning how to use material design icons once I am done I will move to implemting it.

WhatsApp Image 2024-07-17 at 9 38 35 PM
.

@nuilewis
Copy link

Permit me to nitpick please😅 If it's possible to be adjusted, then great... if not then don't waste too much time on it, you can move on to other things.
Design

Thank you @nuilewis . Taking these into consideration.

@nuilewis @Redeem-Grimm-Satoshi here is what I have now. I am still learning how to use material design icons once I am done I will move to implemting it.

WhatsApp Image 2024-07-17 at 9 38 35 PM
.

Much better, good work 👍

@Redeem-Grimm-Satoshi
Copy link
Member

Redeem-Grimm-Satoshi commented Aug 13, 2024

Looks fine

@Chinex-Boroja Please check this out for any testing related issues
@r0ssing Please check this out, It's ready to be merged

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

@Redeem-Grimm-Satoshi @nuilewis @maprehensive
Please I wish to add modifications made on the icons to this pull request since its not yet merged

@juayuohcarineneng19
Copy link
Contributor Author

juayuohcarineneng19 commented Aug 20, 2024

Updated material design icon on image 2
WhatsApp Image 2024-08-20 at 6 44 33 PM

@r0ssing
Copy link
Member

r0ssing commented Aug 21, 2024

Great work! I think these design changes really helps make the application easier to use..
Now that you are working in the 'instances.handlebars' file, is it possible to indent the "Create New Instance" button a little bit so it aligns with the rest of the content? (and maybe remove one of the two horisontal rulers below, so there is only one?)
image

Copy link
Member

@r0ssing r0ssing left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Let me know if you get a chance to also indent the "Create new instance" button, then I will re-review it again - otherwise this is fine to merge... 🥇

}

.instanceRowControl .openInstance {
border-top-left-radius: 8px!important;
Copy link
Member

Choose a reason for hiding this comment

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

Hello - great work! 👍
One small question regarding the CSS; Is the '!important' tag necessary?
It is really a kind of last resort in CSS (i.e. when something "more specific" exists in the css inheritance hierarchy).. However - if it is indeed needed here, then just ignore this comment! 😄

justify-content: center;
align-items: center;
text-align: center
}
Copy link
Member

Choose a reason for hiding this comment

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

indentation issue...

@Redeem-Grimm-Satoshi
Copy link
Member

Great work! I think these design changes really helps make the application easier to use.. Now that you are working in the 'instances.handlebars' file, is it possible to indent the "Create New Instance" button a little bit so it aligns with the rest of the content? (and maybe remove one of the two horisontal rulers below, so there is only one?) image

@r0ssing I guess this was addressed in this PR: #208

It will preferable we merge older PR first before the new ones to avoid mix ups

@r0ssing
Copy link
Member

r0ssing commented Aug 21, 2024

@Redeem-Grimm-Satoshi thanks for pointing this out! In that case I agree, that it will be easier if those are merged first (and backmerged into/updated in this PR)

@Redeem-Grimm-Satoshi
Copy link
Member

@Redeem-Grimm-Satoshi thanks for pointing this out! In that case I agree, that it will be easier if those are merged first (and backmerged into/updated in this PR)

Yes sure

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