#260 Theme and Icons

Merged
mmarif merged 15 commits from light-theme into master 1 month ago
mmarif commented 2 months ago

Will close #249 and #250

Will close #249 and #250
mmarif added this to the 2.4.0 milestone 2 months ago
mmarif added the
Feature
label 2 months ago
mmarif added the
UI/UX
label 2 months ago
mmarif self-assigned this 2 months ago
mmarif changed title from [WIP] Theme and Icons to Theme and Icons 1 month ago
mmarif added the
LGTM-need
label 1 month ago
6543 commented 1 month ago
Collaborator

I’ll have a look :)

I'll have a look :)
6543 started working 1 month ago
6543 reviewed 1 month ago
@@ -16,1 +17,4 @@

final TinyDB tinyDb = new TinyDB(getApplicationContext());

if(tinyDb.getInt("themeId") == 0) {
6543

can you test for == 1 and else set default theme?

can you test for == 1 and else set default theme?
mmarif

Yes, that will work too.

Yes, that will work too.
6543 reviewed 1 month ago
one nit, a question
@@ -113,6 +114,16 @@ public class RepoStargazersAdapter extends BaseAdapter {
viewHolder.memberName.setTypeface(myTypeface);
}

if(tinyDb.getInt("themeId") == 0) { // dark
6543

same as first comment - why not make if (is light) [else if (is other theme - not here jet)] else default -> dark theme

same as first comment - why not make if (is light) [else if (is other theme - not here jet)] else default -> dark theme
mmarif

Yup, you are right.

Yup, you are right.
@@ -1,4 +1,4 @@
<vector android:height="24dp" android:tint="#FFFFFF"
<vector android:height="24dp" android:tint="#368f73"
6543

would be nice if this would be a in the theme config too - but dont know how hard this is to implement ...

would be nice if this would be a in the theme config too - but dont know how hard this is to implement ...
mmarif

Not sure how hard. Haven’t researched on this. But for now the same green color icons matches both themes. Will see if we have more themes later on.

Not sure how hard. Haven't researched on this. But for now the same green color icons matches both themes. Will see if we have more themes later on.
mmarif

Just tried with custom attr and it works. But for now we will keep it as we can improve the themes slowly. Also this PR intended job is done and is very big already.

Just tried with custom attr and it works. But for now we will keep it as we can improve the themes slowly. Also this PR intended job is done and is very big already.
6543

yes have same opinion but nice that is is doable - I’ll create an issue witch links to this comments so we have a record of this :)

yes have same opinion but nice that is is doable - I'll create an issue witch links to this comments so we have a record of this :)
6543

-> #271

-> #271
6543 stopped working 1 month ago
27min
mmarif commented 1 month ago
Owner

Ready for final review. :)

Ready for final review. :)
6543 commented 1 month ago
Collaborator

Cant view diff with GitNex I’ll have to wait tomoroww to look at the pull via PC

--> GitNex crash because of to big diff, can we ad something like paggination?

Cant view diff with GitNex I'll have to wait tomoroww to look at the pull via PC --> GitNex crash because of to big diff, can we ad something like paggination?
mmarif commented 1 month ago
Owner

GitNex crash because of to big diff, can we ad something like paggination?

Currently there is no API for it so I am rendering it from web. I know it is not best at this time and may have problems. But with few files it works. With PR this big it was going to crash/stop work etc.

Room of improvement are very minimal in the current imeplementation tbh. I will leave it as is except fix bugs and wait for the API to be available at later times to have a better implementation of it.

> GitNex crash because of to big diff, can we ad something like paggination? Currently there is no API for it so I am rendering it from web. I know it is not best at this time and may have problems. But with few files it works. With PR this big it was going to crash/stop work etc. Room of improvement are very minimal in the current imeplementation tbh. I will leave it as is except fix bugs and wait for the API to be available at later times to have a better implementation of it.
6543 commented 1 month ago
Collaborator

O didn’t noticed there is an api issue, is there an issue open upstream?

O didn't noticed there is an api issue, is there an issue open upstream?
mmarif commented 1 month ago
Owner

Haven’t looked around but did not come across so far.

Haven't looked around but did not come across so far.
6543 started working 1 month ago
6543 reviewed 1 month ago
@@ -49,2 +50,4 @@
private PDFView pdfView;
private LinearLayout pdfViewFrame;
private byte[] decodedPdf;
private Boolean $nightMode;
6543

why this variable start with $ ?

why this variable start with `$` ?
mmarif

Good catch. I was into PHP mode. :)

Good catch. I was into PHP mode. :)
6543 stopped working 1 month ago
14min 24s
6543 approved these changes 1 month ago
6543 commented 1 month ago
Collaborator

php mode :laughing: - I wonder it works!

I have tested it now - works as expected :+1:

php mode :laughing: - I wonder it works! I have tested it now - works as expected :+1:
6543 added the
LGTM-done
label 1 month ago
6543 removed the
LGTM-need
label 1 month ago
mmarif commented 1 month ago
Owner

I wonder it works!

Haha, java treats it as a character like others.

> I wonder it works! Haha, java treats it as a character like others.
6543 commented 1 month ago
Collaborator

I’m fine with merging this :D

I'm fine with merging this :D
mmarif deleted branch light-theme 1 month ago

Reviewers

6543 approved these changes 1 month ago
The pull request has been merged as d2f5f09c75.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Total Time Spent: 41min 24s
6543
41min 24s
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
Cancel
Save
There is no content yet.