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

JSS manager should support stylesheet functions as instance style overrides #874

Closed
prabashn opened this issue Sep 12, 2018 · 8 comments
Closed
Assignees
Milestone

Comments

@prabashn
Copy link

What is your feature request? Please describe.
Currently for any custom component you can use the manageJss class to add styling attributes to a component. However when a custom component requires to style a child component, we can do so via a custom JSS stylesheet. You can even use IDesignSystem based callbacks to style individual CSS properties. However what seems to be missing is the ability to create an entire JSS stylesheet based on IDesignSystem.

The use case I'm describing is this. Let's say I have a TopSitesAddDialog component in which I'm using the FAST MSFT Dialog component inside. Now from my TopSitesAddDialog TSX, I can do something like where getDialogStyleSheetOverrides will return an object such as { dialog_contentRegion: { [custom overrides] } } in order to override the contentRegion element inside the Dialog. What I want to be able to do is call getDialogStyleSheetOverrides(config: IDesignSystem) so I have access to the IDesignSystem config when I'm creating the stylesheet overrides. If there's a way to get access to IDesignSystem within my TSX component, or some way to inject it inside getDialogStyleSheetOverrides, that would solve the issue for me.

Describe the solution you'd like
I somehow need a way to get access to IDesignSystem when I'm creating the custom style overrides for Dialog's dialog_contentRegion element, for example. One potential solution could be to support both a JSS stylesheet, as well as a (IDesignSystem) => JSS Stylesheet in the jssStyleSheet property, so I can inject my callback function which would give me access to the IDesignSystem.

Describe alternatives you've considered
N/A

Additional context
N/A. @chrisdholt can give additional context.

@triage-new-issues triage-new-issues bot added the status:triage New Issue - needs triage label Sep 12, 2018
@nicholasrice
Copy link
Contributor

Thanks @prabashn - this makes sense. You're looking for something like the following, correct?

function buttonStyleOverrides(designSystem) {
    return {
        // ... button stylesheet here
    };
}

// ...

render() {
    // Render styled button with overrides
   <Button jssStyleSheet={buttonStyleOverrides} />
}

@nicholasrice nicholasrice self-assigned this Sep 17, 2018
@nicholasrice nicholasrice added the feature A new feature label Sep 17, 2018
@triage-new-issues triage-new-issues bot removed the status:triage New Issue - needs triage label Sep 17, 2018
@prabashn
Copy link
Author

prabashn commented Sep 17, 2018

@nicholasrice , exactly. Basically either a direct stylesheet object or a function callback to get the stylesheet based on IDesignSystem. You can add it as a new property, or use the existing property - whatever makes most sense to you and your normal patterns for things like this.

@nicholasrice
Copy link
Contributor

I think keeping the API consistent with the HOC makes the most sense to me - allow jssStyleSheet to accept either a stylesheet object or a function.

@nicholasrice nicholasrice changed the title IDesignSystem based style overrides for child components JSS manager should support stylesheet functions as instance style overrides Sep 19, 2018
@nicholasrice
Copy link
Contributor

Thinking about this more, one issue with function stylesheets all up is that it's possible they return a different value every time, so we currently treat them as being able to have side-effects. This model would pose a problem here because we would have to regenerate styles on every render, because each function call could potentially return a slightly different stylesheet. The perf impact would be pretty bad doing this on render.

We could assert that all stylesheet functions must be pure and code with that assumption. That follows a react model so I don't think that would be a stretch for folks - it would also facilitate splitting static and dynamic styles, which is currently impossible given a stylesheet function.

@chrisdholt @janechu @prabashn thoughts?

@prabashn
Copy link
Author

prabashn commented Sep 19, 2018

This actually came up in my PR yesterday from Brandon. I added you and Chris on the comment. After reflecting upon it, I came to the same conclusion regarding perf.

So basically what I need is the ability to get access to the IDesignSystem instance that's in effect. So really really stupid question, should I just access the current design system instance by doing:

import designConfig from "../design-system";

Because if I can (and should) do this, all I need to do is simply initialize the stylesheet for the dialog outside the component itself, and simply pass that initialized jss style object to the jssStylesheet property. No need for dynamic evaluation (as I assume the design system values don't actually change on the fly - or can they?) Is there a better place within the component itself to initialize the stylesheet for the child components? Maybe in the constructor?

Thoughts? Maybe we don't need this feature after all then, no?

@nicholasrice

@nicholasrice
Copy link
Contributor

Yea I commented on your PR - the design system can change on the fly so I think this is still necessary.

@Falkicon Falkicon added this to the February 2019 milestone Jan 13, 2019
@awentzel awentzel modified the milestones: February 2019, April 2019 Mar 28, 2019
@stale
Copy link

stale bot commented Jul 18, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@chrisdholt
Copy link
Member

closed by #3517

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

No branches or pull requests

6 participants