enonic / app-rss

Apache License 2.0
2 stars 6 forks source link

Updates for 0.11.2 #17

Closed pvmerlo closed 7 years ago

Bellfalasch commented 7 years ago

Nice one =) Good and simple fix for the timezone issue, great!

However, two issues:

pvmerlo commented 7 years ago

"/common/styles/_all.css" is not a base style that's present in all enonic projects? And the build.gradle changes were not compiling (enonic.defaults...) so I've removed it in order to build the app.

Bellfalasch commented 7 years ago

I will check about that /common/-thing, but wasn't aware of it. Not sure it is from us or if it is intentional or stable enough to use.

I will check the gradle thing and get back to you. We have a new way of publishing jars to maven repos and perhaps that is now failing. It shouldn't but since it's new there might be some things we haven't tested.

pvmerlo commented 7 years ago

These admin assets are present on the base Enonic installation and are even referenced on the "Tools" guide on XP docs. So I think this is not a problem... See: http://xp.readthedocs.io/en/stable/developer/admin/tools/index.html

Bellfalasch commented 7 years ago

We are working now internally on solving the issues with enonic.defaults. We must have it to be able to publish the changes. However as you say there is something buggy there and it won't work for you. We will fix this as soon as possible.

I will take a look at the design of the thymeleaf-page. I love the idea, but I might do something with inline style and some text/info anyway.

You don't have to do any changes to this pull request. Just give me some time to fix internal issues and get back to merging and adjusting for 0.11.1!

pvmerlo commented 7 years ago

Ok, thanks for the return!

pvmerlo commented 7 years ago

The v0.11.2 has a initial support to categories, like proposed on #6 (Using RegEx test), the author name like you said on #5, uses the content creator if no author name property is set (supporting only string for now, not an object relationship like on SuperHero Blog does with the author content-type). Also, I've added support to timezone, with basis set on "Etc/UTC" timezone (+00:00) and usable as the Database-like string, like "Europe/Oslo".

sigdestad commented 7 years ago

Hmm.. I assume the timezone setting is only to be used if there is no timezone info available in the data already? http://xp.readthedocs.io/en/stable/developer/schema/input-types/datetime.html

Bellfalasch commented 7 years ago

Hi @pvmerlo nice things.

However, I cannot find why lib-context is added to the build? Also, any reason why not using the moment js lib from Market instead of including it manually?

I will need to do some testing in these changes and the earlier ones. Will also look at the timezone-thing, as @sigdestad says, each content type has it's own setting for timezone on their datetime-field (use or not to use) and that should be used instead of overwriting it from the RSS-content (unnecessary setting). But I will check this all out.

pvmerlo commented 7 years ago

I din't saw the moment lib on market. Sorry :p The context lib was added to solve #15. I'm still working on that. For now you can just remove it? Thanks :)

Bellfalasch commented 7 years ago

Oh ok, perhaps add that fix in another branch so we don't mix done things with half-done things.

I can remove it and take a look at switching out moment just so we don't install dependencies that could be handled through Gradle instead. But also, I need to look at the implementation around timezone in general as it might not always work. However that will be later this week.

Bellfalasch commented 7 years ago

Btw #15 is not planned for 1.0 but placed in a "icebox 2.0"

See https://github.com/enonic/app-rss/projects/1

Bellfalasch commented 7 years ago

I understand these updates are needed asap. To make things go faster, please resolve the following issues in a new pull request:

After that I will merge, tag for 0.12.0 release, publish updated jar, and update market.

Bellfalasch commented 7 years ago

Just taking notes on what is discussed elsewhere:

This will force us to release a 0.12.1 dropping the timezone setting (might be backward breaking), but at least this buys us some more time while critical fixes and features gets released asap.

I will perform this code updates.

Bellfalasch commented 7 years ago

Things are now shipped as 0.12.0 but this all needs a lot of more testing. This is very rushed. Timezone settings doesn't seem to validate in RSS validators. Many readers handle this, but don't count on it. A lot of more thinking and testing is needed around timezone and datetime in general in this app.