devmadhuu commented on code in PR #7048: URL: https://github.com/apache/ozone/pull/7048#discussion_r1713902352
########## hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/components/search/search.tsx: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { Input, Select } from 'antd'; +import { Option } from '@/v2/components/select/singleSelect'; Review Comment: Pls check if this import being used. ########## hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/components/search/search.tsx: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { Input, Select } from 'antd'; +import { Option } from '@/v2/components/select/singleSelect'; + +type SearchProps = { + searchColumn?: string; + searchOptions?: Option[]; + onSearch: ( + arg0: string, + arg1: React.ChangeEvent<HTMLInputElement> + | React.MouseEvent<HTMLElement, MouseEvent> + | React.KeyboardEvent<HTMLInputElement> + | undefined + ) => void; + onChange: ( + value: string, + //OptionType, OptionGroupData and OptionData are not + //currently exported by AntD hence set to any + option: any + ) => void; +} + +const Search: React.FC<SearchProps> = ({ + searchColumn, + searchOptions, + onSearch = () => {}, + onChange = () => {} Review Comment: While you've set default values for `onSearch` and `onChange`, it might be more explicit and clear to set them using default props. This can make the code easier to understand, especially for developers who might not be familiar with default parameters in function signatures. ########## hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/components/search/search.tsx: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { Input, Select } from 'antd'; +import { Option } from '@/v2/components/select/singleSelect'; + +type SearchProps = { + searchColumn?: string; + searchOptions?: Option[]; + onSearch: ( + arg0: string, + arg1: React.ChangeEvent<HTMLInputElement> + | React.MouseEvent<HTMLElement, MouseEvent> + | React.KeyboardEvent<HTMLInputElement> + | undefined + ) => void; + onChange: ( + value: string, + //OptionType, OptionGroupData and OptionData are not + //currently exported by AntD hence set to any + option: any + ) => void; +} + +const Search: React.FC<SearchProps> = ({ + searchColumn, + searchOptions, + onSearch = () => {}, + onChange = () => {} +}) => { + + let selectFilter = null; + + if (searchColumn) { + selectFilter = <Select + defaultValue={searchColumn} + options={searchOptions} + onChange={onChange} /> + } Review Comment: ```suggestion const selectFilter = searchColumn ? ( <Select defaultValue={searchColumn} options={searchOptions} onChange={onChange} /> ) : null; ``` ########## hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/components/search/search.tsx: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { Input, Select } from 'antd'; +import { Option } from '@/v2/components/select/singleSelect'; + +type SearchProps = { + searchColumn?: string; + searchOptions?: Option[]; Review Comment: Both searchColumn and searchOptions are optional (?), but the code does not handle the scenario when searchOptions is not provided. Consider defaulting searchOptions to an empty array to avoid potential issues. ########## hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/components/select/singleSelect.tsx: ########## @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from "react"; +import Select, { + Props as ReactSelectProps, + components, + ValueType, + ValueContainerProps, + StylesConfig +} from 'react-select'; + +import { selectStyles } from "@/v2/constants/select.constants"; + + +export type Option = { + label: string; + value: string; +} + +interface SingleSelectProps extends ReactSelectProps<Option, false> { + options: Option[]; + placeholder: string; + onChange: (arg0: ValueType<Option, false>) => void; +} + +const SingleSelect: React.FC<SingleSelectProps> = ({ + options = [], + placeholder = 'Limit', + onChange = () => { }, + ...props +}) => { + + + const ValueContainer = ({ children, ...props }: ValueContainerProps<Option, false>) => { + const selectedLimit = props.getValue() as Option[] + return ( + <components.ValueContainer {...props}> + Limit: {selectedLimit[0].label} Review Comment: The `selectedLimit[0].label `logic assumes that `selectedLimit` always has at least one selected option. This could lead to errors if `selectedLimit` is empty. Consider adding a conditional check. ```suggestion const selectedLimit = props.getValue() as Option[]; const label = selectedLimit.length > 0 ? selectedLimit[0].label : ''; return ( <components.ValueContainer {...props}> Limit: {label} </components.ValueContainer> ); ``` ########## hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/components/search/search.tsx: ########## @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from 'react'; +import { Input, Select } from 'antd'; +import { Option } from '@/v2/components/select/singleSelect'; + +type SearchProps = { + searchColumn?: string; + searchOptions?: Option[]; + onSearch: ( + arg0: string, + arg1: React.ChangeEvent<HTMLInputElement> + | React.MouseEvent<HTMLElement, MouseEvent> + | React.KeyboardEvent<HTMLInputElement> + | undefined + ) => void; + onChange: ( + value: string, + //OptionType, OptionGroupData and OptionData are not + //currently exported by AntD hence set to any + option: any + ) => void; +} + +const Search: React.FC<SearchProps> = ({ + searchColumn, + searchOptions, + onSearch = () => {}, + onChange = () => {} +}) => { + + let selectFilter = null; Review Comment: Instead of initializing selectFilter with null, you can directly assign it within the if statement. This would make the code slightly more concise: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
