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

Add source pages #1553

Merged
merged 6 commits into from Mar 2, 2022
Merged

Add source pages #1553

merged 6 commits into from Mar 2, 2022

Conversation

jpellizzari
Copy link
Contributor

@jpellizzari jpellizzari commented Mar 1, 2022

Closes #1526
Closes #1520

Adds some new sources:
Screenshot from 2022-03-01 11-51-20

Bucket:
Screenshot from 2022-03-01 11-52-02

Helm
Screenshot from 2022-03-01 11-52-48
Repo (chart was removed, doesn't seem like flux supports it?)

@jpellizzari jpellizzari changed the base branch from main to v2 March 1, 2022 00:07
@jpellizzari jpellizzari changed the title 1526 source pages Add source pages Mar 1, 2022
@jpellizzari jpellizzari marked this pull request as ready for review March 1, 2022 19:57
@jpellizzari jpellizzari requested review from joshri and bia March 1, 2022 19:57

export default styled(Heading).attrs({ className: Heading.name })`
font-size: 20px;
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Text component not flexible enough for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is semantically different. <Text /> is just a <span />. We want the actual <h1 />, <h2 /> etc tags to be there.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about an as prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? It is a semantically different component. I would want all Heading components to have similar styling. I wouldn't want the user to be able to specify bold or not bold, for example. I want all h1 to be bold.

I can make Heading wrap <Text />. Would that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just don't like that it leads to a lot of extra styling for a component where essentially the only thing that changes is the tag - I thought as made it show up semantically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am saying it is a different "module" of code. They do different things. I don't want someone to be able to change the styling of a <Heading /> the same way they can of <Text />. <Heading /> is a way to enforce consistency in this case.

font-size: 20px;
margin-bottom: 12px;
}

h3 {
Copy link
Contributor

@joshri joshri Mar 1, 2022

Choose a reason for hiding this comment

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

what makes this different from <Text fontSize='medium' color='neutral30' /> or something like that

ui/components/SourcesTable.tsx Show resolved Hide resolved
ui/pages/v2/HelmRepositoryDetail.tsx Show resolved Hide resolved
@jpellizzari
Copy link
Contributor Author

@joshri Added some logic to the Heading component to make its purpose clearer. PTAL.

ui/components/Heading.tsx Outdated Show resolved Hide resolved
@jpellizzari jpellizzari force-pushed the 1526-source-pages branch 2 times, most recently from 921c9f5 to 13947c0 Compare March 2, 2022 20:34
@jpellizzari jpellizzari merged commit f1af9fd into v2 Mar 2, 2022
@jpellizzari jpellizzari deleted the 1526-source-pages branch March 2, 2022 20:49
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.

None yet

2 participants