-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: add custom branding with icon, brand name and color #1657
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: alpha
Are you sure you want to change the base?
Conversation
Danger run resulted in 1 warning; to find out more, see the checks page. Generated by 🚫 dangerJS |
return ( | ||
<div className={styles.login} style={{ marginTop: this.props.marginTop || '-220px' }}> | ||
<Icon width={80} height={80} name='infinity' fill='#093A59' /> | ||
{!customBrandIcon && <Icon width={80} height={80} name='infinity' fill='#093A59' />} | ||
{customBrandIcon && <img src={'appicons/' + customBrandIcon} width={80} height={80} alt="Custom BRAND icon"/>} |
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.
Can we put customBrandTitle
into alt
attribute, if available? Otherwise "Brand Logo" should be enough I think.
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.
I changed this.
@404-html the both comments solved. |
@stepanic Thanks for this PR! And apologies that it didn't get much review attention anymore. We are on a spree now to close all open PRs. Could you rebase this on master and resolve any merge conflicts? It would be amazing if we could get this merged! |
That was amazingly fast! I'll review this feature, @404-html do you maybe have any input in the meantime? |
Nothing new from me, thanks for asking. I left some suggestions back in the day but nothing urgent 👍 |
Great! Could you look through the suggestions and see which ones you would mark as resolved? It seems most have been addressed. |
@404-html I see that you left suggestions on 2021-05-19, which are only README related will you commit them and then integrate this PR? |
Sorry @stepanic, looks like I don't have permission: |
@404-html This may be if you had write access at some point. We have recently changes our permission model and are limiting write permissions to reviewers to are active on a regular basis, for security reasons, because the write permissions are quite wide-ranging. @stepanic Could you incorporate the change suggestions and resolve the conversations that have been addressed? Then we can easily see whether there are any open questions. I'm preparing a release, so it would be great to get this feature in there. |
Co-authored-by: 404-html <[email protected]>
Co-authored-by: 404-html <[email protected]>
Co-authored-by: 404-html <[email protected]>
@stepanic There have been some merges on master, could you please rebase/merge and then request a review? |
* upstream/master: docs: Update node in README Update sass and docker (parse-community#1792) ci: Remove parse server dev dependency (parse-community#1796) ci: modernize steps (parse-community#1789) fix(docker): increase node version in docker to 12 (parse-community#1788) Fix: Update CLP for new class (parse-community#1785) feat: Add MFA to Dashboard (parse-community#1624) ci: refactor docker ci (parse-community#1786) ci: Fix docker image pushing to Docker Hub (parse-community#1781)
@mtrezza merged with
|
@stepanic Could you please merge master into this? It again became out-of-date with the base branch. I'll make sure to review this next, so you don't have to do this again. |
* upstream/master: fix: revert parse-community#1706 which introduced new database index requirements for pagination (parse-community#1800)
@mtrezza merged again :D |
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.
This is looking good! A few notes:
-
The path for
customBrandIcon
seems to be hard coded to.../appicons/<file>
. Can you use the path specified in the existingiconsFolder
config option instead? That is the path used for app specific icons, I guess it can also be used for this purpose since they are related. -
Could you remove the
custom...
prefix from the new options, e.g. renamecustomBrandIcon
tobrandIcon
. Any option is inherently a customization. And please also removecustom...
from the code variables to keep the relation between code and feature. -
Could you rename
customBrandTitle
tobrandName
, a "brand name" is a more commonly known term. -
Could you rename
customBrandColorPrimary
tobrandPrimaryBackgroundColor
, similar to the existingprimaryBackgroundColor
for apps. I can imagine that in the future someone may add abrandPrimaryForegroundColor
color to specify the text color. -
There seems to be a layout issue, in the top left title bar:
previously it was:
which was also not optimal, because the title and user are not vertically centered. Maybe, while you are at it, you could fix this and center the text, if not, a future PR can do this. It should be centered, regardless of whether there is a username or not (if you have no
users
set in dashboard config).
@stepanic If you have some time, it would be great if you could look over the feedback above, so we can merge this PR. |
@visualfanatic If you are interested in completing this PR, I think there is not much missing. The feature now has a bounty applied to it since the PR has become stale. |
I would love to see this landing! |
@andreafalzetti Feel free to pick it up if you like, it seems to be almost done. |
parse-dashboard-config.json
is improved with this options:NOTE: favicon and HTML title are on purpose kept unchanged and background color inside of React app.
This is initial version of custom branding, any more advanced custom branding could be implemented in another PR.
From


default
sidebarto
branded
sidebarFrom


default
login formto
branded
login form