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

feat: no iframe, use swagger-ui component #36

Merged
merged 8 commits into from
Nov 7, 2024

Conversation

smarinier
Copy link
Contributor

  • remove iframe use and references
  • remove stoplight from packages
  • add swagger-ui package
  • use SwaggerUI Vue

@provokateurin
Copy link
Member

Thanks, this is a much smaller change than I expected.
Do you also want to add the icons to the sidebar? I think the sidebar looks a lot better with it.

@provokateurin
Copy link
Member

provokateurin commented Nov 5, 2024

For me the dark mode looks terrible:
image
This is the reason I had to use an iframe with elements, it also was broken in dark mode with our CSS.

@smarinier
Copy link
Contributor Author

Thanks, this is a much smaller change than I expected. Do you also want to add the icons to the sidebar? I think the sidebar looks a lot better with it.

I prefer to separate subjects with PRs. I find this make it more readable, and the commit in history get a chance to be significant. So I planned to make another PR for the side bar subject.

Same for the darkmode. It was not done in my app, so i have to work specifically on it. It's not very complicated, as somebody built a SwaggerDark.css.

I remove the view route.

@provokateurin provokateurin linked an issue Nov 5, 2024 that may be closed by this pull request
@provokateurin
Copy link
Member

Same for the darkmode. It was not done in my app, so i have to work specifically on it. It's not very complicated, as somebody built a SwaggerDark.css.

Can you do that in this PR please?
I don't like merging something that is known to be only half-working.

Signed-off-by: Sebastien Marinier <[email protected]>
@provokateurin
Copy link
Member

The dark theme is not working for me, I believe the CSS selector is not matching anything?

@smarinier
Copy link
Contributor Author

smarinier commented Nov 5, 2024

The CSS selector is based on [data-theme-dark], that should be set in your tag (?)

OK. I change for *=dark

Signed-off-by: Sebastien Marinier <[email protected]>
@provokateurin
Copy link
Member

It's still not working for me because it is set to data-themes="".
You should be able to use the logic of the iframe code you removed to set this property correctly.

@smarinier
Copy link
Contributor Author

I'm not use to handle the dark mode. I took selectors that are used in Nextcloud vue components Maybe you should check how you can select a dark theme without having data-themes correctly set ?
For me it's working for both dark modes.

@provokateurin
Copy link
Member

I figured out what the problem is:
If the user configured "System default theme" then we have to follow the general CSS dark mode and not the Nextcloud configured dark mode.
When I explicitly select dark mode in the settings it works as expected.

@smarinier
Copy link
Contributor Author

In that case, you probably found a nextcloud bug.
This CSS selector is used in NcAppNavigation, NcAppNavigationItem and NcDataTimePickerNative.
(Or it's a bug in these components ? )

@provokateurin
Copy link
Member

I'm not sure, it seems they work just fine?

@smarinier
Copy link
Contributor Author

Considering data-themes is not set in your case, i guess this won't work, by example:

// Add extra border for high contrast mode
[data-themes*="highcontrast"] {
.app-navigation {
border-inline-end: 1px solid var(--color-border);
}
}

@provokateurin
Copy link
Member

As far as I can tell everything is correct.
What we need to have is that the styles also apply with [data-themes="default"] and @media (prefers-color-scheme: dark), but unfortunately it is not possible to combine these.
The only way I see is duplicating all styles to match the two cases.

The fact that data-themes can also be an empty string is a bug in the server which I just fixed: nextcloud/server#49112

@provokateurin
Copy link
Member

I know duplicating the styles is a bit ugly, but it works and there is no easy workaround to the problem that you can not combine media queries and selectors.

@smarinier
Copy link
Contributor Author

I think we could avoid the source duplication with sass, but indeed I'm not sure there's a simple way to avoid the double CSS.
We could consider the spent time was usefull if we found a Nextcloud bug ;)
Are they any other points ?

@provokateurin
Copy link
Member

I think we could avoid the source duplication with sass

Sure, would you be able to set that up and change it? I never used it so far...

Are they any other points ?

No I think this good now!

Signed-off-by: Sebastien Marinier <[email protected]>
@smarinier
Copy link
Contributor Author

smarinier commented Nov 6, 2024

I think we could avoid the source duplication with sass

Sure, would you be able to set that up and change it? I never used it so far...

Done! This let me know that the CSS nesting i was using is quite recent with browsers, so this is better, even if the result looks very repetitive.

src/main.js Outdated Show resolved Hide resolved
Signed-off-by: Sebastien Marinier <[email protected]>
Signed-off-by: Sebastien Marinier <[email protected]>
@provokateurin
Copy link
Member

Awesome now it works as expected, except for the server bug 👍

@provokateurin provokateurin merged commit fc29289 into nextcloud:main Nov 7, 2024
15 checks passed
@provokateurin
Copy link
Member

I'll make a new release next week

@smarinier
Copy link
Contributor Author

I'm working on the menu and the routes (#34).
I will provide the PR soon.

@provokateurin
Copy link
Member

Ah ok, then I will wait for those :)

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

Successfully merging this pull request may close these issues.

Spare the authentication to the user
2 participants