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

refactor(web): UI code pruning and clean up round #3 #1540

Merged
merged 19 commits into from
Sep 13, 2024
Merged

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Aug 14, 2024

Apart from a bit of clean up, this PR is intended for start writing better core components that has been on hold for a few months already.

It's the case of core/Page component, which has been rewritten almost for scratch and now makes the weird core/CardField transitioning component obsolete.

Please, note that this set of changes continues with the migration to TypeScript for touched files and also introduce a PatternFly/Flex wrapper in order to ease the work with its responsive props. It's a bit complex because the (ab)use of advanced types but it does the job without introducing props unknown by PF/Flex. As said in the file comments, ideally

would be better to add these responsive props shortcuts direclty in PF/Flex to allow the consumer to just set the default value when not needed to change it depending on the breakpoint. But at this moment we're a bit short of time for creating and testing such an elaborated PR against upstream.

Reviewers: Look at it (core/Flex.tsx) as a PoC to see if we are willing to introduce it and similar ones for the other PatternFly components using responsive props.


Related to #1441 and #1494

Comment on lines +41 to +48
type ResponsiveFlexProps = {
[Key in keyof Omit<FlexProps, "download" | "inlist"> as FlexProps[Key] extends {
default?: unknown;
}
? Key
: // @ts-ignore
never]: FlexProps[Key]["default"] | FlexProps[Key];
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case of doubt, what this advanced type is doing is something like:

  • Asking for all FlexProps and capturing these that somehow looks like a responsive one. I.e., capturing only the ones that are objects with a default optional prop. As said in the comment, don't know why "download" and "inlist" are captured although they does not look like mentioned object. So, just ignore them.
  • If the prop was captured, uses an union of its own type and the type of the default key.
  • If the prop was not captured, just ignore it in the new type by using never.

Copy link
Contributor Author

@dgdavid dgdavid Aug 14, 2024

Choose a reason for hiding this comment

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

don't know why "download" and "inlist" are captured although they does not look like mentioned object.

The problem is that they are typed as any, see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L2923-L3001 (interface FlexProps extends React.HTMLProps, https://github.com/patternfly/patternfly-react/blob/7b64b054146bfffde5bcddda317f2381a6ef540e/packages/react-core/src/layouts/Flex/Flex.tsx#L8)

Sadly (and kind of expected) any matches the above extends constraint. So, would be nice to have a way to ignore the properties of type any but for now it's ok ignoring these two by using the <Omit> helper instead of investing so much time on it. The wrapper works as expected and we could improve it later if finally get merged.

In order to being able to use it with shortcuts for responsive props
instead of having to write the `propName={{ default: "propValue" }}` all
the time.
For replacing the current one by doing things a bit better. Still WIP,
reason why it's in a PageNext.tsx file.
@coveralls

This comment was marked as outdated.

Which still imported from core/PageNext.
Mainly to avoid the tricky workaround for the ReactRouterDom#navigate
overloading. See remix-run/react-router#10505 (comment)
Useful for using the aria-labelledby when no aria-label is given.
By using the PF/Card#component prop.
Instead of an HTML <section> to avoid having too much nested sections.
This change might not be final and changed back again later, once we
have more clear the better structure for a better a11y.
Among other minor improvements, the aria-labelledby attribute is now
added only when needed.
And also use constants for sticky top and bottom values.
And also adds basic unit testing for such a new Page component.
Those .jsx files that were touched in previous commits on the context of
#1540.
Also drop a bunch of not needed `async` keywords in Page.test.tsx
@dgdavid dgdavid marked this pull request as ready for review September 12, 2024 19:51
@dgdavid dgdavid changed the title WIP / refactor(web): UI code pruning and clean up round #3 refactor(web): UI code pruning and clean up round #3 Sep 12, 2024
@dgdavid dgdavid removed the request for review from imobachgs September 12, 2024 20:04
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

I like the new Page component :-)

web/src/components/layout/Flex.tsx Outdated Show resolved Hide resolved
web/src/components/core/PageNext.tsx Outdated Show resolved Hide resolved
web/src/components/core/PasswordAndConfirmationInput.tsx Outdated Show resolved Hide resolved
web/src/components/core/Page.tsx Outdated Show resolved Hide resolved
@dgdavid
Copy link
Contributor Author

dgdavid commented Sep 13, 2024

I like the new Page component :-)

Thanks a lot! I think it's looking better than before, even though there is a lot of work to do yet. But yes, I'm happy with this new Page version, much simpler than before. Also, having the "section" thingy as part of it instead of the "CardField" component helps to write Agama pages IMHO.

web/src/components/layout/Flex.tsx Outdated Show resolved Hide resolved
Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>
@dgdavid dgdavid merged commit 9a6d145 into master Sep 13, 2024
2 checks passed
@dgdavid dgdavid deleted the frontend-cleanup-3 branch September 13, 2024 10:17
dgdavid added a commit that referenced this pull request Sep 13, 2024
Changes sent in #1540 made some
temporary core/Page internal components obsolete, making needing an
adaptation of zFCP pages too.

The commit also updates some copyright headers and drop no longer needed
cspell exceptions.
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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