yardz commented on a change in pull request #13277: URL: https://github.com/apache/superset/pull/13277#discussion_r581462015
########## File path: superset-frontend/src/components/Pagination/index.ts ########## @@ -0,0 +1,26 @@ +/** + * 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 { Next } from './Next'; +import { Prev } from './Prev'; +import { Item } from './Item'; +import { Ellipsis } from './Ellipsis'; +import { PaginationWrapper } from './PaginationWrapper'; Review comment: > should show an error because an arbitrary `childNode` will either render incorrectly or create customization outside of the component itself, defeating the purpose of component abstraction. I agree, that really makes sense. ########## File path: superset-frontend/src/components/Pagination/index.ts ########## @@ -0,0 +1,26 @@ +/** + * 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 { Next } from './Next'; +import { Prev } from './Prev'; +import { Item } from './Item'; +import { Ellipsis } from './Ellipsis'; +import { PaginationWrapper } from './PaginationWrapper'; Review comment: > Re: `PaginationWrapper` vs `Pagination`, it really doesn't matter that much. But when the component is used outside of `createUltimatePagination`, do you really think The second example is really better but at the same time I am bothered to put something like "Pagination.Next = Next" because in my opinion this would need to be tested as well (which is an option). This code also doesn't seem like something that a person with little experience will understand, and it is also more "difficult" to find the original component. I understand that there are benefits but the bad part seems to me to be greater than the benefit. Perhaps changing `PaginationWrapper` to `Container` will improve that part, like this: ```ts import Pagination from 'src/components/pagination'; <Pagination.Container> <Pagination.Prev /> <Pagination.Prev /> </Pagination.Container> ``` It's not that good but it's close... ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
