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

Added Nord based theme #4400

Merged
merged 52 commits into from
Jul 2, 2022
Merged

Added Nord based theme #4400

merged 52 commits into from
Jul 2, 2022

Conversation

joelchrono12
Copy link
Contributor

@joelchrono12 joelchrono12 commented Jun 1, 2022

Closes #

Changes proposed in this pull request:

  • A new theme!-

How to test the feature manually:

  1. Pick it from the settings in Display

I am still kinda noob at this so if anything else is needed please let me know

@Alkarex Alkarex added the Theme label Jun 1, 2022
@Alkarex Alkarex added this to the 1.20.0 milestone Jun 1, 2022
@Alkarex Alkarex requested a review from math-GH June 1, 2022 21:36
@Alkarex
Copy link
Member

Alkarex commented Jun 2, 2022

Hello and thanks for your contribution 👍🏻
I have automatically fixed a number of issues by running make fix-all and make test-all, but there are a few issues remaining (see the output of the automated tests below)

@joelchrono12
Copy link
Contributor Author

joelchrono12 commented Jun 2, 2022

Hello and thanks for your contribution 👍🏻 I have automatically fixed a number of issues by running make fix-all and make test-all, but there are a few issues remaining (see the output of the automated tests below)

Thank you, do you know of how to pull the changes you did to my repo, so I can fix these problems?

@Alkarex
Copy link
Member

Alkarex commented Jun 2, 2022

Thank you, do you know of how to pull the changes you did to my repo, so I can fix these problems?

git pull --ff-only

@math-GH
Copy link
Contributor

math-GH commented Jun 8, 2022

regarding your CSS:

Here you can find the defined order of CSS properties:

"order/properties-order": [
"margin",
"padding",
"background",
"display",
"float",
"max-width",
"width",
"max-height",
"height",
"color",
"font",
"font-family",
"font-size",
"border",
"border-top",
"border-top-color",
"border-right",
"border-right-color",
"border-bottom",
"border-bottom-color",
"border-left",
"border-left-color",
"border-radius",
"box-shadow"
],

@Alkarex
Copy link
Member

Alkarex commented Jun 8, 2022

For the order of attributes and other cosmetic things, running make fix-all automatically fixes all that. I have just done it, so the few remaining errors are those that must be manually handled

@joelchrono12
Copy link
Contributor Author

regarding your CSS:

Here you can find the defined order of CSS properties:

"order/properties-order": [
"margin",
"padding",
"background",
"display",
"float",
"max-width",
"width",
"max-height",
"height",
"color",
"font",
"font-family",
"font-size",
"border",
"border-top",
"border-top-color",
"border-right",
"border-right-color",
"border-bottom",
"border-bottom-color",
"border-left",
"border-left-color",
"border-radius",
"box-shadow"
],

Thanks, I noticed there are some tests which let me know the line numbers where the css is wrong. But as I edit changes those line numbers wrong and I kinda laze around.

Is there a way to run those tests locally? I think I lack some dependencies but idk which ones, sorry if this is in the documentation, I should re-check

@joelchrono12
Copy link
Contributor Author

Well, not a lot of errors left, I'll fixed them now

@Alkarex
Copy link
Member

Alkarex commented Jun 8, 2022

To run tests locally, try make fix-all and make test-all
Otherwise in your case, npm install and npm run fix and npm test will cover most of what you need.
In both cases, nodejs + npm must to be available

@joelchrono12
Copy link
Contributor Author

Well now there are more errors 🙃

@joelchrono12
Copy link
Contributor Author

Hey, it passed, thank you @Alkarex I couldn't manage to get the make running since I still lacked some smaller depenencies. But now its working!

@math-GH
Copy link
Contributor

math-GH commented Jun 8, 2022

Please check the menus in mobile view.

The "user query" submenu is broken (the white rhombus is part of the submenu)

grafik

And the config menu: the close button looks not styled:
grafik

@Alkarex
Copy link
Member

Alkarex commented Jun 18, 2022

Hint 2: We are using https://editorconfig.org/ to specify whitespace policies for various types of files in a standard way. This is supported by most editors, either natively or thanks to a plugin

@joelchrono12
Copy link
Contributor Author

Hint 2: We are using https://editorconfig.org/ to specify whitespace policies for various types of files in a standard way. This is supported by most editors, either natively or thanks to a plugin

Thank you, I installed the plugin for neovim and also removed some configs I had set myself that were conflicting. It should be good now! Sorry

Alkarex added a commit that referenced this pull request Jun 19, 2022
* Document fixes & tests
#fix #4213
Help #4400 (comment)

* Link to tests.yml

* sudo

* Link to GitHub Actions

* Add Fedora and Alpine
Alkarex added a commit that referenced this pull request Jun 19, 2022
* Update all test dependencies

* Remove old false-positive

* Minor update lock files

* Increase PHPStan memory for Fedora
#4400 (comment)

* Require PHP8+ for tests
Due to small changes of signature in `ob_implicit_flush` and `simplexml_load_string`, cf. #4123

* Missing lint in CSS files
@Alkarex
Copy link
Member

Alkarex commented Jun 21, 2022

Please double-check the global view

image

A few things are strange, in particular the background colour of article lists:

image

@joelchrono12
Copy link
Contributor Author

joelchrono12 commented Jun 21, 2022

I cannot replicate this issue, I tried both in chromium and firefox...

@math-GH
Copy link
Contributor

math-GH commented Jun 22, 2022

Please double-check the global view

A few things are strange, in particular the background colour of article lists:

I can confirm this issue with Firefox on Windows

@joelchrono12
Copy link
Contributor Author

joelchrono12 commented Jun 23, 2022

I can confirm this issue with Firefox on Windows

I don't really know how to solve this, my guess is that some some CSS needs an !important added to it. If any of you could find which one is it and add it it would be better since I can't get this to work...

@math-GH
Copy link
Contributor

math-GH commented Jun 23, 2022

I can confirm this issue with Firefox on Windows

I don't really know how to solve this, my guess is that some some CSS needs an !important added to it. If any of you could find which one is it and add it it would be better since I can't get this to work...

fixed

@Alkarex Alkarex merged commit c05ab99 into FreshRSS:edge Jul 2, 2022
@Alkarex
Copy link
Member

Alkarex commented Jul 2, 2022

Thanks again!
Please add a line for you in https://github.com/FreshRSS/FreshRSS/blob/edge/CREDITS.md

@Alwaysin
Copy link
Contributor

I like the theme! 👍

There seems to not be the new message notification banner, is this a feature?

@joelchrono12
Copy link
Contributor Author

I like the theme! 👍

There seems to not be the new message notification banner, is this a feature?

This is based on the default template and it has nothing else added.
When there are new posts available there is a little message showing up a the top of the feed, do you mean that?

@Alwaysin
Copy link
Contributor

Yes, I meant that. Sorry, I probably had a problem on my side, it is now showing correctly!

@Alwaysin
Copy link
Contributor

Alwaysin commented Jul 16, 2022

I think there's definetly a problem with this theme. On latest Chrome version on Windows, my tab of FreshRSS uses less than 200Mo of memory. When I switch to this theme, it uses circa 1Go. That's probably why it sometimes don't show the banner.
It is even more visible on a mobile phone, everything is so slow.

EDIT: Got it, the theme is loading EVERY article. I have 14000+ unread, so yeah.

@math-GH
Copy link
Contributor

math-GH commented Jul 16, 2022

The theme has no effect on the loading behavior. The theme is just the layout (made with CSS).

How to check the resource usage in Google Chrome: https://linuxhint.com/built_in_task_manager_google_chrome/

@Alwaysin
Copy link
Contributor

@math-GH this is the exact tool I've used to check memory status.
When using Nord theme, as soon as I scroll down, it loads every article and memory usage skyrockets. It doesn't do it on other themes.

@math-GH
Copy link
Contributor

math-GH commented Jul 16, 2022

Loading next articles while scrolling: it is a default behavior and does not depending on the theme:
grafik

@Alwaysin
Copy link
Contributor

Maybe I'll do a video of the behaviour if I find time to do so.
I indeed have this feature on, but on other themes, it works as described: it loads at the bottom of te page.
Ths theme, as soon as I scroll, even a millimeter, it loads everything.

@math-GH
Copy link
Contributor

math-GH commented Jul 16, 2022

OK, now I understood and can confirm that behavior.

I created an issue ticket: #4447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants