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

fix(other): copy all constants in backend when branding #6853

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ulfgebhardt
Copy link
Member

@ulfgebhardt ulfgebhardt commented Nov 22, 2023

🍰 Pullrequest

copy all constants in backend when branding, but remove links.ts since not working in backend context.

Issues

Todo

@ulfgebhardt ulfgebhardt changed the title fix(docker): copy all constants in backend when branding fix(other): copy all constants in backend when branding Nov 22, 2023
Copy link
Member Author

@ulfgebhardt ulfgebhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why I was asked to review this even tho its clearly not finished.

There is folders brading-backend and branding-webapp referenced - i could not find them. I guess those are in the branding repositories.

@@ -0,0 +1,5 @@
// this file is duplicated in backend/src/branding/groupsBranding' and 'webapp/branding/groupsBranding'
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this comments. 👍🏼

Comment on lines +7 to +10
export const { DESCRIPTION_WITHOUT_HTML_LENGTH_MIN, DESCRIPTION_EXCERPT_HTML_LENGTH } = {
...groupsDefault,
...groupsBranding,
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why export specific variables? Is there a typescript advantage doing this?

Suggested change
export const { DESCRIPTION_WITHOUT_HTML_LENGTH_MIN, DESCRIPTION_EXCERPT_HTML_LENGTH } = {
...groupsDefault,
...groupsBranding,
}
export default {
...groupsDefault,
...groupsBranding,
}

Copy link
Member

@Tirokk Tirokk Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way because this export is imported as:

import { DESCRIPTION_WITHOUT_HTML_LENGTH_MIN } from '../../constants/groups'

So your suggestion creates an error on this import:

src/middleware/excerptMiddleware.ts:2:10 - error TS2614: Module '"../constants/groups"' has no exported member 'DESCRIPTION_EXCERPT_HTML_LENGTH'. Did you mean to use 'import DESCRIPTION_EXCERPT_HTML_LENGTH from "../constants/groups"' instead?

It's not imported as whole object where a property is selected.

The way I do it has the following good sides:

  • if I go on refactoring I have not to rewrite all the imports
  • it's clear which configs can be passed through and all others are filtered out

That's why I tend to leave my code as it is. Is that all right? @ulfgebhardt

Comment on lines +1 to +2
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this comments. 👍🏼

Comment on lines +2 to +3
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this comments. 👍🏼

Comment on lines +14 to +15
// Wolle console.log('DESCRIPTION_WITHOUT_HTML_LENGTH_MIN: ', DESCRIPTION_WITHOUT_HTML_LENGTH_MIN)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Wolle console.log('DESCRIPTION_WITHOUT_HTML_LENGTH_MIN: ', DESCRIPTION_WITHOUT_HTML_LENGTH_MIN)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I's a work in progress comment. I'll remove it later.

@@ -0,0 +1,5 @@
// this file is duplicated in backend/src/branding/groupsBranding' and 'webapp/branding/groupsBranding'
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this comments. 👍🏼

Comment on lines +181 to +184
// Wolle console.log('NAME_LENGTH_MIN: ', NAME_LENGTH_MIN)
// console.log('NAME_LENGTH_MAX: ', NAME_LENGTH_MAX)
// console.log('DESCRIPTION_WITHOUT_HTML_LENGTH_MIN: ', DESCRIPTION_WITHOUT_HTML_LENGTH_MIN)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Wolle console.log('NAME_LENGTH_MIN: ', NAME_LENGTH_MIN)
// console.log('NAME_LENGTH_MAX: ', NAME_LENGTH_MAX)
// console.log('DESCRIPTION_WITHOUT_HTML_LENGTH_MIN: ', DESCRIPTION_WITHOUT_HTML_LENGTH_MIN)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I's a work in progress comment. I'll remove it later.

Comment on lines +1 to +3
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this comments. 👍🏼

Comment on lines +7 to +12
export const {
NAME_LENGTH_MIN,
NAME_LENGTH_MAX,
DESCRIPTION_WITHOUT_HTML_LENGTH_MIN,
SHOW_GROUP_BUTTON_IN_HEADER,
} = { ...groupsDefault, ...groupsBranding }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const {
NAME_LENGTH_MIN,
NAME_LENGTH_MAX,
DESCRIPTION_WITHOUT_HTML_LENGTH_MIN,
SHOW_GROUP_BUTTON_IN_HEADER,
} = { ...groupsDefault, ...groupsBranding }
export default { ...groupsDefault, ...groupsBranding }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -0,0 +1,8 @@
// this file is duplicated in 'backend/src/constants/groupsDefault' and 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groupsDefault', 'webapp/constants/groupsDefault'
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// configurable values see: 'backend/src/constants/groups', 'webapp/constants/groups'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove this comments. 👍🏼

@Tirokk
Copy link
Member

Tirokk commented Jan 18, 2024

Thanks for your code suggestions! @ulfgebhardt
I will try them.

I don't understand why I was asked to review this even tho its clearly not finished.

There is folders brading-backend and branding-webapp referenced - i could not find them. I guess those are in the branding repositories.

As I explained in our personal conversation I like to know if the concept and the exact code suites you before I refactor everything.
I told you this and said that it's not ready yet.

Yes. The folders config-all, config-backend and config-webapp are only in the branding repositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working devops
Projects
Status: In Progress
2 participants