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

Adjust PLATFORM_NAME setting for Android app compatibility #30

Conversation

Abdul-Muqadim-Arbisoft
Copy link
Contributor

No description provided.

@DawoudSheraz
Copy link
Contributor

This should be merged in either nightly or master.

@Abdul-Muqadim-Arbisoft Abdul-Muqadim-Arbisoft changed the base branch from sumac to master November 7, 2024 07:58
@Abdul-Muqadim-Arbisoft
Copy link
Contributor Author

This should be merged in either nightly or master.

rebased to master

PLATFORM_NAME: "{{ PLATFORM_NAME }}"
# TODO: Temporarily set PLATFORM_NAME based on OPENEDX_COMMON_VERSION to 'OpenEdx' for compatibility with the Android app theme directory
# (https://github.com/openedx/openedx-app-android expects 'OpenEdx'). This is addressed in PR https://github.com/openedx/openedx-app-android/pull/335,
# which is merged into sumac.master and develop. Revert to "PLATFORM_NAME: '{{ PLATFORM_NAME }}'" once the pr changes are merged in master.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "master" branch of android app is actually "main". Please update the comment and then squash the commits.

@@ -68,7 +68,10 @@ BRANCH:
ALTERNATE_HOST: ''

#Platform names
PLATFORM_NAME: "{{ PLATFORM_NAME }}"
# TODO: Temporarily set PLATFORM_NAME based on OPENEDX_COMMON_VERSION to 'OpenEdx' for compatibility with the Android app theme directory
# (https://github.com/openedx/openedx-app-android expects 'OpenEdx'). This is addressed in PR https://github.com/openedx/openedx-app-android/pull/335,
Copy link
Contributor

Choose a reason for hiding this comment

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

(https://github.com/openedx/openedx-app-android expects 'OpenEdx')

Do you have a link that this is the value it expects? The actual issue is that platform name has spaces which cause issues with themeDirs. Therefore, the name is hardcoded to a value with no spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR basically ensures theme works properly without being dependent on the PLATFORM_NAME value, which previously caused issues with theme directories.

@Abdul-Muqadim-Arbisoft
Copy link
Contributor Author

Closing this PR as the changes from PR #335 have already been merged into the main branch. The issue with the 'FIXED' platform name has been resolved with this update.

@Abdul-Muqadim-Arbisoft Abdul-Muqadim-Arbisoft deleted the muqadim/platform-name-fix branch November 12, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Won't fix
Development

Successfully merging this pull request may close these issues.

2 participants