UI improvements and clean ups #1074

Merged
mmarif merged 14 commits from ui-improvements into main 4 months ago
mmarif commented 4 months ago
Owner

This PR mostly intend to improve and re visualize the UI across the app including clean up of layouts and strings.

Some UI changes are experimental like bottom sheet grid layout for repo items and can be extended once mature enough for use.

Few screen shots attached.

Closes #1077
Closes #976

This PR mostly intend to improve and re visualize the UI across the app including clean up of layouts and strings. Some UI changes are experimental like bottom sheet grid layout for repo items and can be extended once mature enough for use. Few screen shots attached. Closes #1077 Closes #976
mmarif added this to the 4.3.0 milestone 4 months ago
mmarif added the
Cleanup
Enhancement
UI/UX
labels 4 months ago
mmarif self-assigned this 4 months ago
mmarif added 3 commits 4 months ago
mmarif added 1 commit 4 months ago
mmarif changed title from UI improvements and clean ups to [WIP] UI improvements and clean ups 4 months ago
mmarif added 1 commit 4 months ago
Collaborator

Whoa, this looks really nice! Few feedback things on the screenshots (only bottom sheet), didn't looked into it further:

  1. The "Create New" label seems to be a bit off. I'd change it to "Create" only.
  2. some names are too long (milestones, collaborators...)
  3. There is too much space in my opinion between the text and the icons if the name is single-line.
Whoa, this looks really nice! Few feedback things on the screenshots (only bottom sheet), didn't looked into it further: 1. The "Create New" label seems to be a bit off. I'd change it to "Create" only. 2. some names are too long (milestones, collaborators...) 3. There is too much space in my opinion between the text and the icons if the name is single-line.
Collaborator

Just did a quick code review, some ideas:

  1. Could you add a selectable background to the different setting option with switches? I.e. let the whole layout respond to the user.
  2. Before releasing 4.3.0, the new bottom sheet layout should be added to the other bottom sheets as well.
Just did a quick code review, some ideas: 1. Could you add a selectable background to the different setting option with switches? I.e. let the whole layout respond to the user. 2. Before releasing 4.3.0, the new bottom sheet layout should be added to the other bottom sheets as well.
Poster
Owner

The "Create New" label seems to be a bit off. I'd change it to "Create" only.

Create should also be fine.

some names are too long (milestones, collaborators...)

Yes, the size of my emulator and in general all emulator like that. On a real phone it will be fine.

There is too much space in my opinion between the text and the icons if the name is single-line.

Make sense, can be reduced a lil.

Could you add a selectable background to the different setting option with switches? I.e. let the whole layout respond to the user.

Ok, let me try.

Before releasing 4.3.0, the new bottom sheet layout should be added to the other bottom sheets as well.

Yes, we will move them slowly to not make PRs big.

> The "Create New" label seems to be a bit off. I'd change it to "Create" only. `Create` should also be fine. > some names are too long (milestones, collaborators...) Yes, the size of my emulator and in general all emulator like that. On a real phone it will be fine. > There is too much space in my opinion between the text and the icons if the name is single-line. Make sense, can be reduced a lil. > Could you add a selectable background to the different setting option with switches? I.e. let the whole layout respond to the user. Ok, let me try. > Before releasing 4.3.0, the new bottom sheet layout should be added to the other bottom sheets as well. Yes, we will move them slowly to not make PRs big.
mmarif added 1 commit 4 months ago
mmarif added 1 commit 4 months ago
mmarif changed title from [WIP] UI improvements and clean ups to UI improvements and clean ups 4 months ago
Poster
Owner

It's ready. I don't want the PR to go beyond a possible check and files. So some changes may come in another PR some time later.

It's ready. I don't want the PR to go beyond a possible check and files. So some changes may come in another PR some time later.
mmarif added 1 commit 4 months ago
Collaborator

Two issues I found:

  1. buttons in AlertDialogs need more space to the rounded corners
  2. The "Create" TV and the separator should be hidden if there are no options below them

And it seems that the switches in settings are not centered - could you center them in the whole layout (including long description)?


Which other dialogs should be update with the new layout (in a followup PR)? To make it consistent, we should update the dialogs on:

  • file viewer
  • user profiles (only (un)follow)
  • your own user profile (only add email)
  • add team members (only add team member)
  • issue bottom sheet (also contains reactions)
  • labels
  • milestones (only close/reopen)
  • tags (only delete)
  • notifications
  • filter options:
    • issues
    • pull requests
    • releases/tags
    • milestones
    • notifications

For bottom sheets with only one option we should consider adding more options (copy URL, open in browser, share...).

Two issues I found: 1. buttons in AlertDialogs need more space to the rounded corners 2. The "Create" TV and the separator should be hidden if there are no options below them And it seems that the switches in settings are not centered - could you center them in the whole layout (including long description)? --- Which other dialogs should be update with the new layout (in a followup PR)? To make it consistent, we should update the dialogs on: * file viewer * user profiles (only (un)follow) * your own user profile (only add email) * add team members (only add team member) * issue bottom sheet (also contains reactions) * labels * milestones (only close/reopen) * tags (only delete) * notifications * filter options: * issues * pull requests * releases/tags * milestones * notifications For bottom sheets with only one option we should consider adding more options (copy URL, open in browser, share...).
Collaborator

Oh, and I wasn't able to click on the "delete repository" option - is this related or was this just a issue of the emulator?

Oh, and I wasn't able to click on the "delete repository" option - is this related or was this just a issue of the emulator?
Poster
Owner

buttons in AlertDialogs need more space to the rounded corners

This should not have happened, I guess I used the global value to increase the round borders. I will revert this change.

The "Create" TV and the separator should be hidden if there are no options below them

Yes, it should be.

For bottom sheets with only one option we should consider adding more options (copy URL, open in browser, share...).

I agree, we should add few basic menu items to all sheets where required. In another PR.

  • copy url
  • open in browser
  • share
  • ?

Oh, and I wasn't able to click on the "delete repository" option - is this related or was this just a issue of the emulator?

No changes are applied to this part. It is working on my side. Must be your emu.

> buttons in AlertDialogs need more space to the rounded corners This should not have happened, I guess I used the global value to increase the round borders. I will revert this change. > The "Create" TV and the separator should be hidden if there are no options below them Yes, it should be. > For bottom sheets with only one option we should consider adding more options (copy URL, open in browser, share...). I agree, we should add few basic menu items to all sheets where required. In another PR. - copy url - open in browser - share - ? > Oh, and I wasn't able to click on the "delete repository" option - is this related or was this just a issue of the emulator? No changes are applied to this part. It is working on my side. Must be your emu.
mmarif added 1 commit 4 months ago
6e008ec927 Merge branch 'main' into ui-improvements
mmarif added 1 commit 4 months ago
mmarif added 1 commit 4 months ago
mmarif added 1 commit 4 months ago
Poster
Owner

All the changes are applied and it's ready.

All the changes are applied and it's ready.
mmarif added 1 commit 4 months ago
qwerty287 reviewed 4 months ago
else if(response.code() == 404) {
if("Pull".equals(issue.getIssueType())) {
Collaborator

if statement is useless.

`if` statement is useless.
mmarif marked this conversation as resolved
viewBinding.issuePrState.setImageResource(R.drawable.ic_issue);
ImageViewCompat.setImageTintList(viewBinding.issuePrState, ColorStateList.valueOf(ctx.getResources().getColor(R.color.iconIssuePrClosedColor)));
} else {
viewBinding.issuePrState.setImageResource(R.drawable.ic_issue);
Collaborator

Maybe add a color setter here as well?

Maybe add a color setter here as well?
mmarif marked this conversation as resolved
context.startActivity(intent);
});
userAvatar.setOnClickListener(loginId -> {
Collaborator

Please make the whole layout/list item clickable.

Please make the whole layout/list item clickable.
Poster
Owner

It is, but somehow clicking on avatar does not trigger it. So I added that. Haven't check why so.

It is, but somehow clicking on avatar does not trigger it. So I added that. Haven't check why so.
mmarif marked this conversation as resolved
public static int returnOnlyNumber(String fileSize) {
Collaborator

Maybe rename to returnOnlyNumberFileSize or something similar?

Maybe rename to `returnOnlyNumberFileSize` or something similar?
mmarif marked this conversation as resolved
android:layout_width="98dp"
android:layout_height="100dp"
android:padding="8dp"
android:background="?android:attr/selectableItemBackground"
Collaborator

For all bottom sheet items: is it possible to use a rounded selectable background? (It's the same on the repo counters for stargazers, watchersm forks, PRs)

For all bottom sheet items: is it possible to use a rounded selectable background? (It's the same on the repo counters for stargazers, watchersm forks, PRs)
mmarif marked this conversation as resolved
mmarif added 1 commit 4 months ago
Poster
Owner

All the changes are done.

All the changes are done.
qwerty287 approved these changes 4 months ago
qwerty287 left a comment
Collaborator

Looks good

Looks good
qwerty287 requested changes 4 months ago
qwerty287 left a comment
Collaborator
  1. On a fresh update to this, the settings switch is set to "on", but the labels are dots only.
  2. If the view is not scrollable (not enough labels), I can't scroll on the labels to switch between the repo tabs. Disable scrolling might help here if possible.
  3. Clicking on the label list does not open the issue detail view.
1. On a fresh update to this, the settings switch is set to "on", but the labels are dots only. 2. If the view is not scrollable (not enough labels), I can't scroll on the labels to switch between the repo tabs. Disable scrolling might help here if possible. 3. Clicking on the label list does not open the issue detail view.
qwerty287 approved these changes 4 months ago
qwerty287 left a comment
Collaborator

Sorry wrong PR.

Sorry wrong PR.
mmarif referenced this issue from a commit 4 months ago
mmarif merged commit 5761c3519e into main 4 months ago
mmarif deleted branch ui-improvements 4 months ago

Reviewers

qwerty287 approved these changes 4 months ago
The pull request has been merged as 5761c3519e.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.