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.

Sign up for GitHub

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

Jump to bottom

feat: use fluentui tokens for theming file and file-list #2044

Merged
merged 40 commits into from
Mar 14, 2023
Merged

feat: use fluentui tokens for theming file and file-list #2044

merged 40 commits into from
Mar 14, 2023

Conversation

musale
Copy link
Contributor

@musale musale commented Feb 15, 2023 •
edited
Loading

Closes #2074
Closes #2075

PR Type

  • Feature

Description of the changes

  • Use fluent UI tokens for theming. In dark/light themes, the component adapts to the inbuilt theme preferences.
  • Prune mgt-file-list to only include tokens that it has created. Tokens from children/borrowed components are available for customizing the child component in use.
  • Update stories to reflect usage of custom CSS tokens to customize the component. This also shows how you can customize a child component.

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

This PR changes CSS tokens that are currently used. It builds on @gavinbarron work to enable theming #2037. It also removes the use of class="mgt-dark" for theming.

@musale musale added this to the 2023-02 milestone Feb 15, 2023
@ghost
Copy link

ghost commented Feb 15, 2023

Thank you for creating a Pull Request @musale.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@musale musale changed the title feat: use fluentui tokens for theming feat: use fluentui tokens for theming file and file-list Feb 15, 2023
@sebastienlevert
Copy link
Contributor

@musale The build is broken, can't use Storybook :(

@musale
Copy link
Contributor Author

musale commented Feb 15, 2023

@musale The build is broken, can't use Storybook :(

updated. Give it a sec 😄

@github-actions
Copy link

The updated storybook is available here

@sebastienlevert
Copy link
Contributor

It's a really good start! I would advise to change the color of the component background in dark as it's the same as the "body" color when in dark. Thoughts?

@Mnickii
Copy link
Collaborator

Mnickii commented Feb 16, 2023

@musale Looks great! @sebastienlevert I think that would change the expected experience when moving from light to dark, as the colors are the same in light mode.

gavinbarron
gavinbarron reviewed Feb 18, 2023
View reviewed changes
Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

This is fantastic work! Love where this is going

packages/mgt-components/src/components/mgt-file/mgt-file.ts Outdated Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.ts Outdated Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.ts Outdated Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.theme.scss Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.scss Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.scss Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.scss Show resolved Hide resolved
packages/mgt-components/src/components/mgt-file-list/mgt-file-list.ts Outdated Show resolved Hide resolved
@musale
fix: set the search icon to be on the same level with the input field (…
5edd7f0 
…#2043)

* fix: set the search icon on the correct level with the input

* fix: update the layout of selected pills
@musale
Merge branch 'next/fluentui' into fix/theme/file
f545006
@github-actions
Copy link

The updated storybook is available here

1 similar comment
@github-actions
Copy link

The updated storybook is available here

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

The updated storybook is available here

@musale
fix: remove focused class usage
be014c5 
Use the :focus and :focus-within selectors
@musale
Copy link
Contributor Author

musale commented Mar 8, 2023 •
edited
Loading

@gavinbarron I made the update. During dev, I had gotten rid of the .focused, I can't remember why I brought it back. I've tested as thoroughly as I could I don't see any dependency to it. Removed in favor of the :focus and :focus-within pseudo classes.

@github-actions
Copy link

github-actions bot commented Mar 8, 2023

The updated storybook is available here

This was linked to issues Mar 8, 2023
Update theming for mgt-file #2074
Closed
Update theming for mgt-file-list #2075
Closed
@sebastienlevert
Copy link
Contributor

Should we remove the background color on mgt-file? We shouldn't provide a background color IMO.

@github-actions
Copy link

The updated storybook is available here

1 similar comment
@github-actions
Copy link

The updated storybook is available here

@sebastienlevert
Copy link
Contributor

We should remove the Dark Mode stories

@musale
Copy link
Contributor Author

musale commented Mar 10, 2023

We should remove the Dark Mode stories

We have an issue for that #2085 to remove them for all components at once.

@ghost ghost added the Needs: Author Feedback Issue needs response from issue author label Mar 13, 2023
@github-actions
Copy link

The updated storybook is available here

@ghost ghost removed the Needs: Author Feedback Issue needs response from issue author label Mar 14, 2023
@github-actions
Copy link

The updated storybook is available here

@Mnickii Mnickii mentioned this pull request Mar 14, 2023
Merged
6 tasks
gavinbarron
gavinbarron approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@gavinbarron gavinbarron left a comment

Choose a reason for hiding this comment

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

Great work 🚀

@gavinbarron gavinbarron merged commit 89b79e9 into next/fluentui Mar 14, 2023
@gavinbarron gavinbarron deleted the fix/theme/file branch March 14, 2023 15:39
This was referenced Jul 17, 2023
Closed
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavinbarron gavinbarron gavinbarron approved these changes

@Mnickii Mnickii Mnickii approved these changes

@sebastienlevert sebastienlevert Awaiting requested review from sebastienlevert

Assignees
No one assigned
Labels
Build: Storybook
Projects
Graph Toolkit
Archived in project
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

Update theming for mgt-file-list Update theming for mgt-file
4 participants
@musale @sebastienlevert @Mnickii @gavinbarron