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

srt new icon refs to #52746 #594

Merged
merged 4 commits into from
Nov 7, 2023
Merged

srt new icon refs to #52746 #594

merged 4 commits into from
Nov 7, 2023

Conversation

aurreco-uga
Copy link
Member

@aurreco-uga aurreco-uga commented Nov 4, 2023

i need to add a hook to access the current build/release number (here harcoded).. but this is working!
Screen Shot 2023-11-04 at 5 30 33 PM

Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

This looks good, but I'm not sure about the newBuild property on RadioList. It seems a little too specific to WDK features.

Maybe we could add a new component named WdkFeature that takes an optional buildNumber property, and performs the logic of including the icon? I'm just thinking of a way to maximize reuse.

You can get the current build with a hook like this:

function useCurrentBuildNumber(): number | undefined {
  return useWdkService(async wdkService => {
    const config = await wdkService.getConfig();
    return config.buildNumber;
  })
}

and can be used like this:

// type is `number | undefined`
const currentBuild = useCurrentBuild();

@jernestmyers
Copy link
Contributor

I opted for a low-cost solution for now.

@jernestmyers jernestmyers requested a review from dmfalke November 7, 2023 16:58
@jernestmyers jernestmyers merged commit a12ee26 into main Nov 7, 2023
1 check passed
@jernestmyers jernestmyers deleted the srt-new-icon branch November 7, 2023 20:15
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.

3 participants