-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[Google translate] Add a preference for the default action #12078
[Google translate] Add a preference for the default action #12078
Conversation
Thank you for your contribution! 🎉 🔔 @gebeto @FezVrasta @ickas @metakirby5 @tangerine1202 @nirtamir2 @pernielsentikaer @rasitayaz you might want to have a look. |
3be18b6
to
6bfd2e5
Compare
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.
Hi 👋
Thanks for your contribution 💪
I have now tested your extension, and I have some feedback ready for you 🙂
"include": ["src/**/*", "raycast-env.d.ts"],
I'm looking forward to testing this extension again 🔥
Request a new review when you are ready. Feel free to contact me here or at Slack if you have any questions.
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.
let's merge it only after proper refactoring
53f6619
to
8651980
Compare
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.
this is a last thing 😅 Looks great!
@@ -42,16 +43,15 @@ export default function Translate(): ReactElement { | |||
const tooltip = `${langFrom?.name ?? langFrom?.code} -> ${langTo?.name ?? langTo?.code}`; | |||
|
|||
return ( | |||
<> | |||
<React.Fragment key={index}> |
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.
@gebeto Just curious why it's needed 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.
<></>
is a shorthand for <React.Fragment></React.Fragment>
, when we are assigning the key
, we need to use React.Fragment
, because we can't do <key={index}>
we need a key
right on fragment here because it is an element that we are returning from the map
. Key was used on nested elements before, and you could see an error in the console:
More details about key
you can get in official react docs here: https://react.dev/learn/rendering-lists
And here is also more details about Fragments
and keys
: https://react.dev/reference/react/Fragment#rendering-a-list-of-fragments
long story short - key
should be applied to the root element which is returning from the map
, in this case it is a Fragment
Minor code style fixes.
8651980
to
0c64e0d
Compare
@pernielsentikaer I think we are good to merge here 🙌 |
Published to the Raycast Store: |
🎉 🎉 🎉 We've rewarded your Raycast account with some credits. You will soon be able to exchange them for some swag. |
Description
This PR adds the ability to specify the default action for the translation:
Screencast
Checklist
npm run build
and tested this distribution build in Raycastassets
folder are used by the extension itselfREADME
are placed outside of themetadata
folder