Skip to content

Add new skin #512

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
Merged
merged 1 commit into from
Oct 4, 2021
Merged

Add new skin #512

merged 1 commit into from
Oct 4, 2021

Conversation

sakkamade
Copy link
Contributor

@sakkamade sakkamade commented Sep 30, 2021 •
edited
Loading

Sorry for opening it at such an early stage, but I have a few questions.

Some examples:

Click me

image

image

sakkamade
sakkamade commented Sep 30, 2021
View reviewed changes
martinrotter
martinrotter reviewed Sep 30, 2021
View reviewed changes
@martinrotter martinrotter self-assigned this Sep 30, 2021
@martinrotter martinrotter added this to the 4.0.4 milestone Sep 30, 2021
@martinrotter martinrotter added Component-GUI Type-Enhancement This is request for brand new feature. labels Sep 30, 2021
@sakkamade
Copy link
Contributor Author

sakkamade commented Sep 30, 2021 •
edited
Loading

Ahahaha. I only now realised how in-place the theme name looks. ❤️

@sakkamade
Copy link
Contributor Author

sakkamade commented Sep 30, 2021 •
edited
Loading

So, I did some fixes, also fixed indentations, and changed 4- to 2-space indent level for you.
Though it looks a bit messy now; not sure why you prefer it.

Still WIP.

@sakkamade
Copy link
Contributor Author

sakkamade commented Sep 30, 2021 •
edited
Loading

By the way, I would recommend to move comment section from html wrapper to a separate file, as it introduces bugs with certain feeds. Such as doubled articles, because, somehow, they ignore that that part (with all other %2) is commented out.

@sakkamade
Copy link
Contributor Author

I will squash all commits later, so as to easier review changes now.

@sakkamade sakkamade force-pushed the the-theme branch 2 times, most recently from a3c1db7 to 6de8b46 Compare October 1, 2021 04:20
@martinrotter
Copy link
Owner

By the way, I would recommend to move comment section from html wrapper to a separate file, as it introduces bugs with certain feeds. Such as doubled articles, because, somehow, they ignore that that part (with all other %2) is commented out.

You are completely right, fixing.

@sakkamade sakkamade force-pushed the the-theme branch 2 times, most recently from eded5c4 to 90026f9 Compare October 4, 2021 04:49
@sakkamade
Copy link
Contributor Author

@martinrotter, That's it I think.
Please review latest commit(s) and let me know when I can squash everything.

@martinrotter
Copy link
Owner

All looks fine to me. You do not have to squash, it will squash when merged. Let me know when you are ready to merge.

@sakkamade
Copy link
Contributor Author

sakkamade commented Oct 4, 2021 •
edited
Loading

You do not have to squash, it will squash when merged.

Will it? I meant squash my commits. Like this:

force-pushed the the-theme branch from 90026f9 to 20c4c78

@sakkamade
Copy link
Contributor Author

Let me know when you are ready to merge.

It is all ready... I guess.

@sakkamade sakkamade marked this pull request as ready for review October 4, 2021 05:31
@martinrotter martinrotter merged commit 3da397a into martinrotter:master Oct 4, 2021
@martinrotter martinrotter added the Status-Fixed Ticket is resolved. label Oct 4, 2021
@sakkamade sakkamade deleted the the-theme branch October 4, 2021 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinrotter martinrotter martinrotter left review comments

Assignees

@martinrotter martinrotter

Labels
Component-GUI Status-Fixed Ticket is resolved. Type-Enhancement This is request for brand new feature.
Projects
None yet
Milestone
4.0.4
Development

Successfully merging this pull request may close these issues.
2 participants
@sakkamade @martinrotter