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

Adding more branding customization options #1078

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sumedhm
Copy link

@sumedhm sumedhm commented Jul 12, 2023

Adding following options for branding:

  1. Adding customLogoText - Use text directly instead of customLogoSvg.
  2. New boolean hideInfoAndFeedback - Hide info & feedback buttons from header & side bar.

Works on local, will need help adding tests.

Docs - https://github.com/allegro/turnilo/pull/1078/files#diff-810bf688e1aff85985ea4b886f0ffcb3dcb7a46d9e199db78ae63a6a561147a1R64

Results

Sidebar - 'Analytics Demo' is the value of customization.customLogoText. 'Info & Feedback' link hidden.
image

image Header - 'Analytics Demo' is the value of `customization.customLogoText`. 'Info & Feedback' on right side hidden.

@sumedhm sumedhm requested a review from a team as a code owner July 12, 2023 11:12
Sumedh Masulkar added 2 commits July 13, 2023 12:18
@mkuthan
Copy link
Member

mkuthan commented Jul 13, 2023

Could you tell, why do you want to hide information about Turnilo licensing and source?
Apache License 2.0 defines that developers must include the original copyright notice, a copy of the license text itself, and in some cases, a copy of the notice file with attribution notes and a disclosure of any significant changes made to the original code.

Info and feedback menu is a convenient way to do that.

@sumedhm
Copy link
Author

sumedhm commented Jul 13, 2023

@mkuthan Our use case for this would be a non commercial but external facing portal, which will be used by our clients to do their analysis - which might be the same for many other Turnilo users.
We would be happy to include a copyright notice, and even mention Turnilo somewhere on the page (bottom or header with a different text), Info & Feedback would be a little misleading for external customers, as it is not Info or feedback for us directly, but it's Info & Feedback for Turnilo.

@mkuthan
Copy link
Member

mkuthan commented Jul 14, 2023

@mkuthan Our use case for this would be a non commercial but external facing portal, which will be used by our clients to do their analysis - which might be the same for many other Turnilo users. We would be happy to include a copyright notice, and even mention Turnilo somewhere on the page (bottom or header with a different text), Info & Feedback would be a little misleading for external customers, as it is not Info or feedback for us directly, but it's Info & Feedback for Turnilo.

Gr8 :) If the "Info & Feedback" is misleading we could fix it instead of hiding. I'm open for the discussion about better placement and content of "Info & Feedback" dialog.

@sumedhm
Copy link
Author

sumedhm commented Jul 14, 2023

Sure @mkuthan :), can we rename it to 'About Turnilo' permanently? If yes, I can do the changes for both the places, and remove the hide config. Although removing it from the header, and having it in the side navbar is something, that would work best for us.

@adrianmroz
Copy link
Collaborator

The svg logo is inlined in the configuration file, couldn’t you just inline their desired text in svg in text node? You’d get more freedom because you could also in-line some styles.

@sumedhm
Copy link
Author

sumedhm commented Jul 15, 2023

The svg logo is inlined in the configuration file, couldn’t you just inline their desired text in svg in text node? You’d get more freedom because you could also in-line some styles.

As a developer, I found it easy to code this much rather than design that svg. I would say this config is more convenient.

@sumedhm
Copy link
Author

sumedhm commented Jul 17, 2023

@adrianmroz @mkuthan
Does that sound good?

  1. Rename Info & Feedback to 'About Turnilo', remove from the header, have it in the sidebar.
  2. Add the customLogoText config.

@adrianmroz-allegro
Copy link
Contributor

As a developer, I found it easy to code this much rather than design that svg. I would say this config is more convenient.

It is hardly a design task:

customization:
  customLogoSvg: |
    <svg>
      <text y="20" font-size="20" font-weight="bold" fill="hsl(21, 100%, 50%)">
        Analytics Demo
      </text>
    </svg>

I am against adding additional flags that conflict with previously defined. New configuration options puts a lot of burden on maintainers.

@mkuthan
Copy link
Member

mkuthan commented Jul 17, 2023

The svg logo is inlined in the configuration file, couldn’t you just inline their desired text in svg in text node? You’d get more freedom because you could also in-line some styles.

As a developer, I found it easy to code this much rather than design that svg. I would say this config is more convenient.

What if both options would be configured? Show warning, thrown an error or silently skip one of the settings?
I vote for inline SVG, good example in the documentation should solve the svg design issue.

@sumedhm
Copy link
Author

sumedhm commented Aug 2, 2023

@adrianmroz-allegro @adrianmroz
Thank you for the suggestion. I tried that out, the sidebar has the logo but the header still says Turnilo, I think orgs would want their product name in both the places. We want it to say {Company} Analytics and not Turnilo as this is not only for internal consumers.
Another option - Make the changes to apply the svg logo on header too.

@mkuthan We should silently skip one with documentation clearly mentioning which config takes precedence.

Is it okay if I rename Info & Feedback to About Turnilo or should I keep that as it is with hideInfoAndFeedback control?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants