new configuration values for open desk environment - #127
Conversation
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
🦋 Changeset detectedLatest commit: a978d54 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
| @@ -0,0 +1,90 @@ | |||
| { | |||
There was a problem hiding this comment.
I understand these temporary files are useful for testing the new design but I think they should not be part of the repository, so let's not forget to remove them once this is completed.
| @@ -0,0 +1,13 @@ | |||
| <!doctype html> | |||
There was a problem hiding this comment.
Same as the navigation.json, these should be used for testing and removed before merging.
| ? 'neoboard' | ||
| : '' | ||
| : 'Whiteboard' | ||
| : (roomNameState?.entities[roomId]?.content.name ?? 'neoboard'); |
There was a problem hiding this comment.
The same logic would also apply here - in the very uncommon case of a room without a name, the text shown here should also be "Whiteboard" if running on openDesk.
Ideally, I'd rather have a REACT_APP_PRODUCT_NAME env that defaults to 'NeoBoard` and uses that here, like:
roomId === undefined
? productName
: (roomNameState?.entities[roomId]?.content.name ?? productName);
where
const productName = getEnvironment(
'REACT_APP_PRODUCT_NAME',
'NeoBoard',
);
and it would be override to Whiteboard on openDesk deployments
| banner: { | ||
| ics_navigation_json_url: getEnvironment( | ||
| 'REACT_APP_OPENDESK_BANNER_ICS_NAVIGATION_JSON_URL', | ||
| `${window.location.origin}/tmp/navigation.json`, |
There was a problem hiding this comment.
we should not default to a temporary file here - the env variables should be mandatory.
There was a problem hiding this comment.
these are just for PR purposes. the same goes for the hardcoded files regarding navigation and silent.html
| ), | ||
| ics_silent_url: getEnvironment( | ||
| 'REACT_APP_OPENDESK_BANNER_ICS_SILENT_URL', | ||
| `${window.location.origin}/tmp/silent.html`, |
| }); | ||
|
|
||
| it('logs in silently', () => { | ||
| it.todo('logs in silently', () => { |
There was a problem hiding this comment.
because the build fails on navbar if we set the loggedin state to true. which we need for the navigation to simulate opendesk env. and if that is set to true the tests fail. but these also would be reverted
| const { t, i18n } = useTranslation(); | ||
| const [ariaExpanded, setAriaExpanded] = useState(false); | ||
| const [loggedIn, setLoggedIn] = useState(false); | ||
| const [loggedIn, setLoggedIn] = useState(true); |
There was a problem hiding this comment.
needs to change back to false, I assume this was for testing?
| '#f5f8fa', | ||
| ); | ||
|
|
||
| // opendesk navbar styling |
There was a problem hiding this comment.
these // opendesk nabber styling comments being repeated on every property don't add any value - we either have like a section separator comment indicating that all the above properties are specific to openDesk styling or take time to specify how each property affects the style.
| // opendesk navbar styling | ||
| const backgroundHover = getEnvironment( | ||
| 'REACT_APP_OPENDESK_BANNER_BACKGROUND_COLOR_HOVER', | ||
| '#f5f8fa', |
There was a problem hiding this comment.
We should go thru all the defaults as I think they are not fully matching the Figma design.
|
|
||
| export function getEnvironmentAppearance(): string { | ||
| return getEnvironment('REACT_APP_APPEARANCE', 'neoboard'); | ||
| return getEnvironment('REACT_APP_APPEARANCE', 'opendesk'); |
There was a problem hiding this comment.
I think we should not change the default appearance to be openDesk.
There was a problem hiding this comment.
true! but again it is for testing purposes for the PR
…of navbar Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net> Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
Signed-off-by: nurjinn jafar <nurjin.jafar@nordeck.net>
with some extra hardcoded values for testing purposes to emulate opendesk environment
neoboard.appearance.mov
opendesk.appearance.mov
✔️ Checklist
Signed-off-byline in the message (more info).