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

[WIP] File Name Extension Add #1673

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AkshatSrivastava2
Copy link

@AkshatSrivastava2 AkshatSrivastava2 commented May 28, 2018

File Name Field Add. Unable to change the file name.

Fixes #1666

@hzoo
Copy link
Member

hzoo commented May 28, 2018

Deploy preview for babel-new ready!

Built with commit 5694645

https://deploy-preview-1673--babel-new.netlify.com

@@ -81,7 +81,7 @@ export default function compile(code: string, config: CompileConfig): Return {
try {
const babelConfig = {
babelrc: false,
filename: "repl",
filename: config.fileName,
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer we name this config.filename in lowercase so it would be more consistent with Babel's own option name.

Copy link
Member

Choose a reason for hiding this comment

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

On the sample page, this appears to not be set. Not sure what the issue is. Maybe it's not getting passed along somewhere?

@@ -215,6 +217,15 @@ class ExpandedContainer extends Component<Props, State> {
/>
File Size
</label>
<label className={styles.settingsLabel}>
File Name
<input
Copy link
Member

Choose a reason for hiding this comment

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

At least on the published sample, it doesn't seem like I'm actually able to type in this box?

@alexzherdev
Copy link
Contributor

@AkshatSrivastava2 are you still working on this? I can pick up if necessary, otherwise you can check out #1676 (which I think is similar) for hints. cc @loganfsmyth

@AkshatSrivastava2
Copy link
Author

@alexzherdev If you could help me with where am I doing wrong would be great. It will be my first contribution in the open source community.
Otherwise, you may wish to proceed with the issue. :)

@babel-bot
Copy link
Contributor

babel-bot commented Jul 19, 2018

Deploy preview for babel failed.

Built with commit aeb20d1

https://app.netlify.com/sites/babel/deploys/5b50bcac792f894e8105a43d

Copy link
Contributor

@alexzherdev alexzherdev left a comment

Choose a reason for hiding this comment

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

Added some comments. Also, you're still missing a change necessary in PluginConfig.js (see my change https://github.com/babel/website/pull/1676/files#diff-70f5c2fe2441a48519c15305524aef72)

@@ -0,0 +1,36 @@
<component name="ProjectCodeStyleConfiguration">
Copy link
Contributor

Choose a reason for hiding this comment

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

All these files in .idea/ should not be committed

@@ -154,6 +154,7 @@ require("@babel/parser").parse("code", {
> NOTE: When a plugin is specified multiple times, only the first options are considered.

- `decorators`:
- `decoratorsBeforeExport` (`boolean`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm why is this change here?

@@ -71,6 +71,7 @@ type State = {
pluginSearch: ?string,
showOfficialExternalPlugins: boolean,
loadingExternalPlugins: boolean,
filename: ?string,
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to add the setting to the state initializer similar to https://github.com/babel/website/pull/1676/files#diff-13734ac24709204bbae97c4b659d1ba1R149

@@ -213,6 +215,15 @@ class ExpandedContainer extends Component<Props, State> {
/>
File Size
</label>

<label className={styles.settingsLabel}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This label is missing a closing tag after the input, hence the broken build.

@@ -213,6 +215,15 @@ class ExpandedContainer extends Component<Props, State> {
/>
File Size
</label>

<label className={styles.settingsLabel}>
File Name
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be indented one level deeper

<label className={styles.settingsLabel}>
File Name
<input
type= "text"
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after = isn't necessary

type= "text"
className={styles.inputFileBoxLeft}
size={fileNameBoxSize}
value={filename}
Copy link
Contributor

Choose a reason for hiding this comment

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

For this controlled component to work properly, it needs to have an onChange handler to report the changes happening to it. You can probably follow how I did it https://github.com/babel/website/pull/1676/files#diff-ecb6fb48984cd148bfc45fb63ca3f34aR225

@@ -80,6 +80,7 @@ export type PluginState = LazyLoadedState & {

export type PluginStateMap = { [name: string]: PluginState };

export type filename = "file.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this type, I believe what this is saying is that a filename can only be equal to file.js, which isn't what we want.

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.

5 participants