-
Notifications
You must be signed in to change notification settings - Fork 287
💄Generic Theme #792
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
base: main
Are you sure you want to change the base?
💄Generic Theme #792
Conversation
c51d9da
to
edfbe9f
Compare
734e834
to
106a5e5
Compare
This should be documented, the same way I suggest on the css PR. |
106a5e5
to
84b5e2f
Compare
84b5e2f
to
3492ea2
Compare
33c7b01
to
6eb903f
Compare
6eb903f
to
0982285
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
0982285
to
2e34dba
Compare
By default Docs will not be on the dsfr theme but on the generic theme. La Gaufre is part of the dsfr theme and is removed from the generic theme. Same for the "beta" keyword and the "proconnect" buttons.
The content of the footer is now configurable via a JSON file. The JSON file is loaded from the FRONTEND_PATH_JSON_FOOTER environment variable. It is probably not the final solution, but it is a first step to make the footer configurable. In the future, we will probably have a backend service to manage the footer content.
Legal pages are not needed anymore in the application. In the dsfr instances, the legal pages will be displayed on a Docs pages. We let the users of Docs managing the legal pages on their own instances.
We want to desaturate the images system in the generic theme to make them less colorful and more in line with the overall theme. We added a special class to the images that need to be desaturated. Other property then desaturated can be apply depending on the theme.
The favicons were still with the dsfr color. We added the generic favicon in the assets folder. The favicon can be a url loaded from the theme, so when Drive will be running, we will be able to store the dsfr favicons there, and remove them from the repo.
2e34dba
to
d150e4d
Compare
@@ -63,6 +63,5 @@ COLLABORATION_SERVER_SECRET=my-secret | |||
COLLABORATION_WS_URL=ws://localhost:4444/collaboration/ws/ | |||
|
|||
# Frontend | |||
FRONTEND_THEME=default | |||
FRONTEND_FOOTER_FEATURE_ENABLED=True | |||
FRONTEND_URL_JSON_FOOTER=http://frontend:3000/contents/footer-demo.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FRONTEND_URL_JSON_FOOTER=http://frontend:3000...
Should be localhost
not frontend
- right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.. so the issue is that the backend container now needs to address the frontend container with the internal hostname. Because
- by default we put test-jsons in the frontend assets
- the backend makes the request to the json here (not recommended)
Problems:
- Developing on localhost without the frontend-container is not possible.
- Confusing when configuring. The perspective suddenly is different.
- It adds complexity that is hard to maintain -> not scalable
I would advise keeping it simple stupid and make the requests from the frontend for now.
Configuring the components via Django Admin should be concern of:
#842
const { data: footerJson } = useFooter(); | ||
|
||
useEffect(() => { | ||
if (!footerJson) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
footerJson was not available for me when i tested.
I set FRONTEND_URL_JSON_FOOTER.
An error message should be added:
useEffect(() => {
if (!footerJson) {
console.error('Error: Footer JSON is not available');
return;
}
import { FooterType } from '../types'; | ||
|
||
export const getFooter = async (): Promise<FooterType> => { | ||
const response = await fetchAPI(`footer/`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The extra backend api for the footer is not necessary.
- It will get messy and hard to maintain if we do this for multiple components.
- Caching on the backend is not that useful because a request still has to be made.
- Better to cache the json on the client and remove the backend endpoint to keep it simple
- We can add an universal config validation later with the admin form to configure components
I would advise to remove it and cache the json on the client instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, you know what?
I'm working on a PoC to improve how componets are configured anyways.
I'll be using the Footer as the PoC example and i can fix all the changes i requested myself.
Lets merge it for now, because this MR is really big and it will be a PITA to get conflicts resolved later.
I've deployed this theme on my server, and after using it, I think it lacks of colours... |
I guess @soyouzpanda is right. Maybe using the blue as primary color on buttons not just on hoover? what do you think @rl-83 ? |
Purpose
The DSFR theme has some elements not compatible with the open source usage, like the Marianne Font, La Gauffre, the Marianne Logo, the footer.
Proposal
New settings
New settings about the footer are now available:
📝 See the documentation: https://github.com/suitenumerique/docs/blob/8f3939cf9eb1072fd39fcf47c69dd05cf6483edc/docs/theming.md#footer-configuration-
Demo