search

nextcloud / bookmarks

🔖 Bookmark app for Nextcloud
https://apps.nextcloud.com/apps/bookmarks
GNU Affero General Public License v3.0
1.02k stars 173 forks source link

visual issues if using other themes #596

Closed Lyterio closed 5 years ago

Lyterio commented 5 years ago
### Steps to reproduce 1. Install and activate your own Theme (for example https://github.com/mwalbeck/nextcloud-breeze-dark) in your nextcloud 2. navigate to bookmarks app 3. click on a bookmark to open details ### Expected behaviour The "bookmark-detail slide-in" and dropdown list (select2) should look like the main theme. See files app. This has the same detail slide in and also tags and dropdown. I would expect that the bookmark app would use existing css classes from nextcloud instead of creating own styles. ### Actual behaviour Bookmark app has its own css styles even for styles which could be used from nextcloud for example select2.css. Why not use nextcloud css to get unified look and feel? Also makes it hard for creating own themes because of additional / redundant css. ### Server configuration

Nextcloud version: 14.0.3 Bookmarks version: 0.14.2

marcelklehr commented 5 years ago

Bookmark app has its own css styles even for styles which could be used from nextcloud for example select2.css.

That's true. The reason for this is that the bookmarks app comes with its own version of select2 which is incompatible with the one used by nextcloud.

As far as I know, the sidebar hasn't been standardized yet. See https://docs.nextcloud.com/server/14/developer_manual/design/index.html for example, where it doesn't show up. Thus I don't think the css selectors used in the files app are treated as a public API, which would make using them very problematic. I'm always open for suggestions or corrections, though.

Lyterio commented 5 years ago

Hi marcelklehr,

That's true. The reason for this is that the bookmarks app comes with its own version of select2 which is incompatible with the one used by nextcloud.

They look similar and do the same. In which way are they incompatible? Even if the functionality were different at least the css styles could be used from nextcloud.

I don't think this has to be described in the dev manual. The guidelines can't cover 100% anyways.

Reusing existing code is common sense and should be best practice. So if there is the same functionality in one of nc's core apps (files with sidebar and tags) this should be reused as much as possible.

Check the developer manual css. Creating your own theme can be hard if apps have their own css classes or elements. Reinventing something does increase the spent time and maintenance.

Just wanted to inform that this can be improved. It's up to you weather to do it or not. Thanks for this great app!

marcelklehr commented 5 years ago

That's true. The reason for this is that the bookmarks app comes with its own version of select2 which is incompatible with the one used by nextcloud.

They look similar and do the same. In which way are they incompatible? Even if the functionality were different at least the css styles could be used from nextcloud.

They are incompatible on an API consumer level, which means it is not trvial to replace one with the other, but it can be done.

Reusing existing code is indeed a good practice, I agree with you, however, software is bound to change. With that comes the responsibility of declaring the likelihood of changes on specific parts and the caution to not use parts directly that weren't declared for public usage, because they are likely to change. While it is often convenient to reuse code it is not always future-proof to do so.

If the bookmarks app used the styles of the files app, it would depend on them staying exactly as they are now in future releases, but as they aren't even documented this is not likely in my opinion.

Thank you for taking the time to provide this feedback. Creating a theme for nextcloud at present is indeed a hard task, in my opinion. Precisely because lots of things are not unified and can't be unless they are standardized. I'll leave this issue open and get back to this, once the sidebar standardization is done.

Lyterio commented 5 years ago

Missing standardizations is already known and someone seems to be working on it: Standardisation summary #8374 Sidebar standardization #10289 There is hope! 😄

mat-m commented 5 years ago

Bookmarks is not mentioned in #8374.

Does someone plans to fix values with respect to vars, at least ?

marcelklehr commented 5 years ago

This shoud be resolved with the upcoming v2.1.0 -- kindly shout or open a new issue, if there are still obstacles left. :)