Skip to content
This repository was archived by the owner on Jul 16, 2021. It is now read-only.

manage repositories from API and UI#335

Merged
prydonius merged 7 commits into
helm:masterfrom
prydonius:257-add-repositories
Aug 15, 2017
Merged

manage repositories from API and UI#335
prydonius merged 7 commits into
helm:masterfrom
prydonius:257-add-repositories

Conversation

@prydonius
Copy link
Copy Markdown
Member

@prydonius prydonius commented Aug 14, 2017

  • adds API methods for creating and deleting repositories
  • adds basic UI view for managing repositories

screen shot 2017-08-14 at 20 28 02
screen shot 2017-08-14 at 20 28 19

- adds API methods for creating and deleting repositories
- adds basic UI view for managing repositories
@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 14, 2017

Codecov Report

Merging #335 into master will decrease coverage by 1.57%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
- Coverage   89.98%   88.41%   -1.58%     
==========================================
  Files          18       17       -1     
  Lines         769      820      +51     
==========================================
+ Hits          692      725      +33     
- Misses         48       61      +13     
- Partials       29       34       +5
Impacted Files Coverage Δ
src/api/handlers/repos/repos.go 69.01% <66.66%> (+5.37%) ⬆️
src/api/config/cache.go 100% <0%> (ø) ⬆️
src/api/config/cors/cors.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab03fb7...7c71c35. Read the comment docs.

}
// CSS from https://codepen.io/AllThingsSmitty/pen/MyqmdM
table {
border: 1px solid #ccc;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please, use the colors defined in theme.scss or a variation of them (lighten, darken...)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I might need help from this, I was trying to import theme.scss but was getting an error. I'll try again and ping you on Slack!

width: 100%;
table-layout: fixed;
}
table caption {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we don't need this rule. Are we going to use captions?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No probably not, I just blanket copied the CSS from the site, will clean this up a bit!

font-size: 1.5em;
margin: .5em 0 .75em;
}
table tr {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Enclose the properties using SCSS:

table {
  border: ...;
  ...
  
  tr {
    ...
  }
}

table caption {
font-size: 1.3em;
}
table thead {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the purpose of this? I don't know why we can't just usedisplay: none;

) {}

ngOnInit() {
this.mdIconRegistry.addSvgIcon(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we using the search, close and menu icons? I only see the delete icon in the template

&__form {
&__errors {
margin: 1em 0;
color: #f00;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use colors from theme.scss

Comment thread src/ui/src/app/shared/models/repo.ts Outdated
name: string;
URL: string;
source: string;
name: string = "";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we are using single quotes in the repo. Just consistency :)

@Angelmmiguel
Copy link
Copy Markdown
Member

A part of the comments, how does look the project in a mobile browser with the new element in the navbar?

@prydonius
Copy link
Copy Markdown
Member Author

screen shot 2017-08-15 at 10 41 58

@Angelmmiguel it overflows a bit at a certain point. I was considering just leaving it as is and have @kemcake or you clean it up after, but if you prefer to have this fixed beforehand I can have a try.

@Angelmmiguel
Copy link
Copy Markdown
Member

The code LGTM now. However, we must solve the header issue. It's fine if you do it in a separate PR. Up to you :)

@prydonius prydonius merged commit 13f7c27 into helm:master Aug 15, 2017
@prydonius prydonius deleted the 257-add-repositories branch August 15, 2017 16:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants