Add external links page section #141

epapbak · 2023-06-28T07:56:38Z

Implements issue #95.

TODO:

Need to find better logos for external cloud providers
Refactor common code
Add tests

F-X64 · 2023-06-28T14:14:40Z

Hey @epapbak thanks for getting this done!
I think we're already pretty close. A couple of things I've noticed:

The flex layout you're using for the provider cards is aligning the content on the left side. The proposal had the content centered with the left card taking 1/3 and the right box 2/3 of the horizontal space with each cards content centered. My flexbox / grid layout knowledge is somewhat limited @KKoukiou @lucasgarfield could you assist on this? I'll gladly share our wireframe with you.
I don't know what the purpose of the empty LinkHeader.ts file is. Can we remove this?

src/app/Pages/Dashboard/Home.tsx

KKoukiou · 2023-06-28T14:29:43Z

src/app/components/ExternalLinksCard.tsx

+  return (
+    <Flex direction={{default: 'column'}} flex={{default: 'flex_3'}}
+          justifyContent={{default: 'justifyContentSpaceBetween'}}
+          style={{border: '1px solid #D2D2D2', padding: '2%'}}>


Ugh I find inline styles pretty ugly. That's a preference obviously - but I think code is much cleaner, if you use CSS files for adding the styles, and use classNames for the selectors.

I'm not super familiar with Typescript best practices but @epapbak if you want to go this route I would be okay with it.
@KKoukiou what is considered good style, adding them into a single app.css or should this be split into component css files?

I prefer one CSS file per page, but if you have very little custom CSS (which should be your goal) we can keep the whole CSS in a single file.

Just a very general note I'd like to add, as I agree with @KKoukiou about the goal of as little custom CSS as possible...

One thing that I have learned about working with designers is that they will usually happily defer to Patternfly's established patterns.

When I find myself in a situation where I would need a lot of custom CSS to match a design, I'll usually try to figure out the 'standard' way to do it in Patternfly and then go back to the designer and propose we use their pattern. That almost always leads to everyone being happy, and I don't have to write any CSS.

Here, the custom CSS is actually a red flag to me... It might be totally necessary, I don't know, but I'm eager to look at the designs to confirm.

I've tried to use different layouts offered by Patternfly, but the end result was always slightly different from what I expected.

For example, aligning the "Build an image ->" link with the other links was pretty much impossible without using Flex layout with justifyContent={{default: 'justifyContentSpaceBetween'}} and alignSelf={{default: "alignSelfStretch"}} on each child. But I'm a beginner with this framework, so there might be something I missed while trying it out...

Regarding the layout, I wonder if you might have more luck using a grid?

https://www.patternfly.org/v4/layouts/grid#with-gutters

You should be able to specify a span of 4 for the first card and 8 for the second card.

You might still run into an issue here where it doesn't behave well at different viewport widths, though... To be honest I'm not 100% sure what the best solution is but I would look into the Patternfly layouts... the grid, the level, or the split could all be potential options.

https://www.patternfly.org/v4/layouts/level
https://www.patternfly.org/v4/layouts/split

I did spend an entire afternoon trying to do it with the Grid and Split layouts. Didn't try Level though. I'll give it a go when I have some time, see if I can make it work without additional styling.

Oof, yes alignment can be tricky. There's a reason centering a div is a meme... 😭 😉

If I can find some time this afternoon I'll try a few things out, too.

This seems to work reasonably well:

<Grid hasGutter> <GridItem md={4} sm={12}> <InternalLinksCard /> </GridItem> <GridItem md={8} sm={12}> <ExternalLinksCard /> </GridItem> </Grid>

src/app/components/InternalLinkCard.tsx

KKoukiou · 2023-06-28T14:34:08Z

Hey @epapbak thanks for getting this done! I think we're already pretty close. A couple of things I've noticed:

* The flex layout you're using for the provider cards is aligning the content on the left side. The proposal had the content centered with the left card taking 1/3 and the right box 2/3 of the horizontal space with each cards content centered. My flexbox / grid layout knowledge is somewhat limited @KKoukiou @lucasgarfield could you assist on this? I'll gladly share our wireframe with you.

* I don't know what the purpose of the empty LinkHeader.ts file is. Can we remove this?

Sure - please share the expected view (mockup) and some instructions on how to build this ideally, so that I can try this out.

F-X64 · 2023-06-28T14:46:40Z

Hey @epapbak thanks for getting this done! I think we're already pretty close. A couple of things I've noticed:
* The flex layout you're using for the provider cards is aligning the content on the left side. The proposal had the content centered with the left card taking 1/3 and the right box 2/3 of the horizontal space with each cards content centered. My flexbox / grid layout knowledge is somewhat limited @KKoukiou @lucasgarfield could you assist on this? I'll gladly share our wireframe with you.

* I don't know what the purpose of the empty LinkHeader.ts file is. Can we remove this?
Sure - please share the expected view (mockup) and some instructions on how to build this ideally, so that I can try this out.

I've send you the sketch file. You can run the project after "npm install" either through "npm run start:dev" or "npm run build && npm start" for the production build.

epapbak · 2023-06-29T07:13:22Z

Hey @epapbak thanks for getting this done! I think we're already pretty close. A couple of things I've noticed:

The flex layout you're using for the provider cards is aligning the content on the left side. The proposal had the content centered with the left card taking 1/3 and the right box 2/3 of the horizontal space with each cards content centered. My flexbox / grid layout knowledge is somewhat limited @KKoukiou @lucasgarfield could you assist on this? I'll gladly share our wireframe with you.

I don't know what the purpose of the empty LinkHeader.ts file is. Can we remove this?

the empty LinkHeader.ts --> my bad, I started refactoring and pushed it without noticing. Removing =)

lucasgarfield · 2023-06-29T15:35:51Z

src/app/components/ExternalLinksCard.tsx

+  return (
+    <Flex direction={{default: 'column'}} flex={{default: 'flex_3'}}
+          justifyContent={{default: 'justifyContentSpaceBetween'}}
+          style={{border: '1px solid #D2D2D2', padding: '2%'}}>


So it seems to me that this styling is really just re-implementing a card. What if you used a card from Patternfly here instead?

https://www.patternfly.org/v4/components/card/

Now, the card does have a shadow and your design does not. There is a 95% chance that this is fine. This is where if you tell your designer "Hey, you know the PF card has a shadow by default!" there is a good chance they will say "Oh, okay, let's go with that!".

If they say "No, no shadow here for {REASONS}", then you can use CSS (for the card!) to remove the border. The PF documentation has the specific CSS selectors:
https://www.patternfly.org/v4/components/card/#css-variables

Actually, regarding the shadow, all you need to do is pass in the isFlat prop and it will look exactly like the designs! https://www.patternfly.org/v4/components/card#modifiers

lucasgarfield · 2023-06-29T17:42:38Z

src/app/components/ExternalLinksCard.tsx

+
+            <>
+              <FlexItem key={`provider_card_${name}`}
+                        style={{borderRight: index != providerInfos.length - 1 ? '1px solid #979797' : 'null'}}>


This is another good example of where you should start questioning yourself when you start writing custom styles... which you should avoid as much as possible! 😉 In this case, you're re-implementing a divider... check it out here:

https://www.patternfly.org/v4/components/divider/#vertical-in-flex-layout

My advice is that anytime you start writing custom styles, ask yourself, is there an existing Patternfly component that does this? 99% of the time, there is an existing Patternfly pattern that the designers will defer to. That's their gold standard and what they will refer to themselves to answer these kinds of questions.

You can also probably use a <Card> here as well, just pass the isPlain prop in to remove the border.

lucasgarfield · 2023-06-30T06:52:59Z

src/app/components/InternalLinkCard.tsx

+    <Flex direction={{default: 'column'}} flex={{default: 'flex_1'}}
+          justifyContent={{default: 'justifyContentSpaceBetween'}}
+          alignSelf={{default: "alignSelfStretch"}}
+          style={{border: '1px solid #D2D2D2', padding: '2%'}}>


Here I think you ought to also use the Card component:
https://www.patternfly.org/v4/components/card

lucasgarfield · 2023-06-30T06:58:18Z

src/app/components/ExternalLinksCard.tsx

+const providerInfos = [
+  {
+    name: 'aws',
+    text: 'Launch in AWS ->',


There's an icon in Patternfly for the arrow you might be able to use, search for fa-arrow-right:
https://www.patternfly.org/v4/guidelines/icons/#all-icons

lucasgarfield · 2023-07-03T12:22:16Z

I’ll be on holiday for the next two weeks so this is the last feedback I have for a while! I was actually able to make a lot of progress on the layout. I recommend using <Card> components in a <Grid>. You can do something like this:

      <PageSection variant={PageSectionVariants.light}>
        <Grid hasGutter>
          <GridItem md={4} sm={12}>
            <InternalLinksCard />
          </GridItem>
          <GridItem md={8} sm={12}>
            <ExternalLinksCard />
          </GridItem>
        </Grid>
      </PageSection>

For the external links card, getting the icons to work well is a bit trickier. I settled on taking this approach, using <Flex>:

const loadImage = (name: Provider) => {
  switch (name) {
    case Provider.aws:
      return aws_logo;
    case Provider.azure:
      return azure_logo;
    case Provider.google:
      return google_logo;
  }
};

const displayText = (name: Provider) => {
  switch (name) {
    case Provider.aws:
      return 'Launch in AWS';
    case Provider.azure:
      return 'Launch in Azure';
    case Provider.google:
      return 'Launch in Google Cloud';
  }
};

enum Provider {
  'aws',
  'azure',
  'google'
}

type ProviderCardProps = {
    provider: Provider;
  }

const ProviderCard = ({ provider }: ProviderCardProps) => {
  return (
              <Stack>
              <StackItem>
              <Bullseye>
                <img style={{ height: 100, width: 100 }} src={loadImage(provider)} />
              </Bullseye>
              </StackItem>
              <StackItem>
              <Bullseye>
              <Button variant="link" isLarge>
                {displayText(provider)} <ArrowRightIcon />
              </Button>
              </Bullseye>
              </StackItem>
              </Stack>
            )
}

const ExternalLinksCard: React.FunctionComponent = () => {
  return (
    <Card isLarge isFlat>
      <CardTitle>
      <Stack>
      <StackItem>
        <img
          src={redhat_logo}
          style={{
            height: '2vw',
          }}/>
      </StackItem>
      <StackItem>
        <p>Get the latest Red Hat Enterprise Linux certified image for cloud deployment</p>      
      </StackItem>
      </Stack>
      </CardTitle>
      <CardBody>
      <p>Launch the latest RHEL certified cloud image through available cloud service provider marketplaces.</p>
      <br/>
    <Flex justifyContent={{ default: 'justifyContentCenter'}}>
    <FlexItem>
      <ProviderCard provider={Provider.aws} />
    </FlexItem>
    <FlexItem>
      <ProviderCard provider={Provider.google} />
    </FlexItem>
    <FlexItem>
      <ProviderCard provider={Provider.azure} />
    </FlexItem>
    </Flex>
      </CardBody>
    </Card>
  );
};

For the internal links card, I took this approach:

const InternalLinksCard: React.FunctionComponent = () => {
  return (
    <Card isFlat isFullHeight isLarge>
      <CardTitle>
      <Stack>
      <StackItem>
        <img
          src={redhat_logo}
          style={{
            height: '2vw',
          }}/>
      </StackItem>
      <StackItem>
        <p>Build your own RHEL cloud image</p>      
        </StackItem>
      </Stack>
      </CardTitle>
      <CardBody>With Red Hat's Image Builder, you can create customizable and repeatable operating system images and server images.</CardBody>
      <CardFooter>
    <Button variant="link" isLarge>
      Build an image <ArrowRightIcon />
    </Button>
      </CardFooter>
    </Card>
  );
};

There are still a few small things to do here and there. You’ll need to remove the padding on the left from the “Build an image →” link. And I have not yet added <Divider> components in-between the cloud provider icons.

I actually think the dividers are going to be quite difficult. How does it behave at small viewports? Should the icons stack vertically? How does the transition from horizontal to vertical occur? I feel like what I have now works pretty well. Maybe you can ask the designer if the <Divider> is really required? Here’s how what I’m proposing works:

We actually have a very similar pattern in the Image Builder Wizard, and we don’t use dividers for the cloud icons. So that is an established pattern you can point to, for consistency’s sake.

I think the code snippets above should be enough to get the idea but you can check out my branch here as well, apologies for the mess on the code formatting and commits 😉:

https://github.com/lucasgarfield/cloud-image-directory-frontend/tree/external-links

F-X64 · 2023-07-03T12:33:16Z

@lucasgarfield I really like that look! This is way more mobile friendly then the rest of the page.

epapbak · 2023-07-19T14:48:28Z

I’ll be on holiday for the next two weeks so this is the last feedback I have for a while! I was actually able to make a lot of progress on the layout. I recommend using <Card> components in a <Grid>. You can do something like this:

      <PageSection variant={PageSectionVariants.light}>
        <Grid hasGutter>
          <GridItem md={4} sm={12}>
            <InternalLinksCard />
          </GridItem>
          <GridItem md={8} sm={12}>
            <ExternalLinksCard />
          </GridItem>
        </Grid>
      </PageSection>

For the external links card, getting the icons to work well is a bit trickier. I settled on taking this approach, using <Flex>:

const loadImage = (name: Provider) => {
  switch (name) {
    case Provider.aws:
      return aws_logo;
    case Provider.azure:
      return azure_logo;
    case Provider.google:
      return google_logo;
  }
};

const displayText = (name: Provider) => {
  switch (name) {
    case Provider.aws:
      return 'Launch in AWS';
    case Provider.azure:
      return 'Launch in Azure';
    case Provider.google:
      return 'Launch in Google Cloud';
  }
};

enum Provider {
  'aws',
  'azure',
  'google'
}

type ProviderCardProps = {
    provider: Provider;
  }

const ProviderCard = ({ provider }: ProviderCardProps) => {
  return (
              <Stack>
              <StackItem>
              <Bullseye>
                <img style={{ height: 100, width: 100 }} src={loadImage(provider)} />
              </Bullseye>
              </StackItem>
              <StackItem>
              <Bullseye>
              <Button variant="link" isLarge>
                {displayText(provider)} <ArrowRightIcon />
              </Button>
              </Bullseye>
              </StackItem>
              </Stack>
            )
}

const ExternalLinksCard: React.FunctionComponent = () => {
  return (
    <Card isLarge isFlat>
      <CardTitle>
      <Stack>
      <StackItem>
        <img
          src={redhat_logo}
          style={{
            height: '2vw',
          }}/>
      </StackItem>
      <StackItem>
        <p>Get the latest Red Hat Enterprise Linux certified image for cloud deployment</p>      
      </StackItem>
      </Stack>
      </CardTitle>
      <CardBody>
      <p>Launch the latest RHEL certified cloud image through available cloud service provider marketplaces.</p>
      <br/>
    <Flex justifyContent={{ default: 'justifyContentCenter'}}>
    <FlexItem>
      <ProviderCard provider={Provider.aws} />
    </FlexItem>
    <FlexItem>
      <ProviderCard provider={Provider.google} />
    </FlexItem>
    <FlexItem>
      <ProviderCard provider={Provider.azure} />
    </FlexItem>
    </Flex>
      </CardBody>
    </Card>
  );
};

For the internal links card, I took this approach:

const InternalLinksCard: React.FunctionComponent = () => {
  return (
    <Card isFlat isFullHeight isLarge>
      <CardTitle>
      <Stack>
      <StackItem>
        <img
          src={redhat_logo}
          style={{
            height: '2vw',
          }}/>
      </StackItem>
      <StackItem>
        <p>Build your own RHEL cloud image</p>      
        </StackItem>
      </Stack>
      </CardTitle>
      <CardBody>With Red Hat's Image Builder, you can create customizable and repeatable operating system images and server images.</CardBody>
      <CardFooter>
    <Button variant="link" isLarge>
      Build an image <ArrowRightIcon />
    </Button>
      </CardFooter>
    </Card>
  );
};

There are still a few small things to do here and there. You’ll need to remove the padding on the left from the “Build an image →” link. And I have not yet added <Divider> components in-between the cloud provider icons.

I actually think the dividers are going to be quite difficult. How does it behave at small viewports? Should the icons stack vertically? How does the transition from horizontal to vertical occur? I feel like what I have now works pretty well. Maybe you can ask the designer if the <Divider> is really required? Here’s how what I’m proposing works:

We actually have a very similar pattern in the Image Builder Wizard, and we don’t use dividers for the cloud icons. So that is an established pattern you can point to, for consistency’s sake.

I think the code snippets above should be enough to get the idea but you can check out my branch here as well, apologies for the mess on the code formatting and commits 😉:

https://github.com/lucasgarfield/cloud-image-directory-frontend/tree/external-links

next level code reviewing 😮 !!
I played around with grids and levels and couldn't make anything close to this, so I'll be borrowing this code 😉

lucasgarfield · 2023-07-20T11:21:55Z

Thanks @epapbak, glad you like it! 😀

Let me know when it's ready for a second look, and don't hesitate to ping me if you get stuck on something.

F-X64 · 2023-07-26T11:14:27Z

@epapbak Just a quick status check. Is this ready for a final review or do you need more time?

epapbak · 2023-07-26T14:21:59Z

It is ready for review, yes :)

F-X64

Looking good, thanks for getting this done!
@lucasgarfield @KKoukiou anything left that needs changing?
Otherwise I'd vote for creating follow up issues and getting this in.

epapbak · 2023-07-27T08:57:41Z

Looking good, thanks for getting this done!
@lucasgarfield @KKoukiou anything left that needs changing?
Otherwise I'd vote for creating follow up issues and getting this in.

Yep, ever growing pull requests (plus my messed up availability) might block some other work, so I'd prefer follow-up issues too.

lucasgarfield · 2023-07-28T10:37:35Z

LGTM! I agree that any issues that arise can be dealt with in follow-up PRs. Great work @epapbak. 😁

epapbak force-pushed the external_links_page_section branch from 1e0ce42 to ae6e492 Compare June 28, 2023 07:57

F-X64 requested review from F-X64 and KKoukiou June 28, 2023 09:13

epapbak linked an issue Jun 28, 2023 that may be closed by this pull request

Add page section for external links #95

Closed

F-X64 requested a review from lucasgarfield June 28, 2023 13:33

KKoukiou reviewed Jun 28, 2023

View reviewed changes

epapbak force-pushed the external_links_page_section branch 2 times, most recently from c690eac to 3b8167b Compare June 29, 2023 07:35

lucasgarfield reviewed Jun 29, 2023

View reviewed changes

lucasgarfield reviewed Jun 30, 2023

View reviewed changes

Add External links page section

a50546f

epapbak closed this Jul 19, 2023

epapbak force-pushed the external_links_page_section branch from 3b8167b to 47f0278 Compare July 19, 2023 15:21

epapbak reopened this Jul 19, 2023

F-X64 approved these changes Jul 27, 2023

View reviewed changes

F-X64 merged commit 5df2332 into main Aug 1, 2023

F-X64 deleted the external_links_page_section branch August 1, 2023 07:38

Add external links page section #141

Add external links page section #141

Uh oh!

Conversation

epapbak commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

F-X64 commented Jun 28, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasgarfield Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KKoukiou commented Jun 28, 2023

Uh oh!

F-X64 commented Jun 28, 2023

Uh oh!

epapbak commented Jun 29, 2023

Uh oh!

lucasgarfield Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucasgarfield commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

F-X64 commented Jul 3, 2023

Uh oh!

epapbak commented Jul 19, 2023

Uh oh!

lucasgarfield commented Jul 20, 2023

Uh oh!

F-X64 commented Jul 26, 2023

Uh oh!

epapbak commented Jul 26, 2023 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

F-X64 left a comment

Choose a reason for hiding this comment

Uh oh!

epapbak commented Jul 27, 2023

Uh oh!

lucasgarfield commented Jul 28, 2023

Uh oh!

Uh oh!

epapbak commented Jun 28, 2023 •

edited

Loading

lucasgarfield Jun 30, 2023 •

edited

Loading

lucasgarfield Jun 29, 2023 •

edited

Loading

lucasgarfield commented Jul 3, 2023 •

edited

Loading

epapbak commented Jul 26, 2023 via email •

edited

Loading