-
Notifications
You must be signed in to change notification settings - Fork 106
feat(many): refactor theme parser and implement tray and tabs #2297
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
Conversation
|
|
build:themes currently breaks, when design fixes the issue, a rebase will resolve it in our end RESOLVED |
| import type { TabsTabProps, TabsTabStyle } from './props' | ||
|
|
||
| type StyleParams = { | ||
| loaded: boolean |
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 don't think loaded is used here.
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.
copy+paste mistake, thanks for catching it
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.
@HerrTopi You forgot to remove 'loaded' from here
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.
Removed, this time for real :D
ToMESSKa
left a comment
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.
see my comments
|
the upgrade guide should be updated too as some tokens were renamed |
matyasf
left a comment
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.
See my comments.
| ) | ||
|
|
||
| const componentFileContent = ` | ||
| subcomponents.forEach(async (subcomponent, subcomponentIndex) => { |
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.
You should use here a for...of, so the return value of the promises is not ignored
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 don't think this should be an issue in this case. What return value do I attempt to use anyways?
My main reason not to change this is that I need the index as well and if I use the for-of pattern, I need to set up a counter.
Please tell me if I misunderstand something
| import type { TabsTabProps, TabsTabStyle } from './props' | ||
|
|
||
| type StyleParams = { | ||
| loaded: boolean |
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.
@HerrTopi You forgot to remove 'loaded' from here


TEST_PLAN:
run
pnpm run build:themes. Check the resulting data. It now should parse jsons with multiple components and subcomponents properly. An example isTabs. It hasTabs.PanelandTabs.Tabas subcomponents. It should generate 3 files from that json: tabs, tabsPanel and tabsTab.All componentID based override (from the docs) should work as before, without change.
the change also contains the migration of tabs and tray, check with the desing documentation