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

Container detail page #304

Merged
merged 36 commits into from
Apr 8, 2021

Conversation

ZitaNemeckova
Copy link
Member

@ZitaNemeckova ZitaNemeckova commented Feb 16, 2021

https://issues.redhat.com/browse/AAH-250

TODO:

  • Add tabs
  • Detail tab - README editor and display
  • Activity tab
  • Activity table
  • Heat map - followup PR
  • Instructions
  • Images tab/table
  • Empty states
  • Images API
  • Activity API
  • Readme API
  • truncate image digest correctly
  • Instruction for container registry
  • Instructions for image

Screenshot 2021-03-30 at 15 10 52

Screenshot 2021-03-30 at 15 12 16

Screenshot 2021-03-30 at 15 11 04

EMPTY STATES:
Screenshot 2021-03-12 at 14 50 56
Screenshot 2021-03-12 at 14 35 16
Screenshot 2021-03-12 at 14 35 23

Sorry, something went wrong.

@sbuenafe-rh
Copy link

@ZitaNemeckova on the header there should be an Edit button (to edit the repo). The description should be place in a tooltip (black bkg).

I'll look into the ReadMe empty state message and button placement. But for the time being, I don't believe you need the "Please add README" text above the Save & Cancel action buttons.

@ZitaNemeckova
Copy link
Member Author

ZitaNemeckova commented Mar 2, 2021

@sbuenafe-rh Thanks for review :) I want to add Edit button with a modal form in followup PR to make it easier to review codewise.

Fixed:
Screenshot 2021-03-02 at 11 02 55
Screenshot 2021-03-02 at 11 03 05

@sbuenafe-rh
Copy link

@ZitaNemeckova tooltip looks good! thanks for the note on the other Edit button with modal. I'll try to draft up an empty state message for the readme.

@ZitaNemeckova ZitaNemeckova force-pushed the container_detail_page branch from 83ad142 to b925cf3 Compare March 12, 2021 13:52
@ZitaNemeckova ZitaNemeckova changed the title [WIP] Container detail page Container detail page Mar 17, 2021
@ZitaNemeckova ZitaNemeckova force-pushed the container_detail_page branch from 551061c to 934952b Compare March 17, 2021 13:56
}

.editor {
// I don't know why, but just setting width causes this element to get
Copy link
Collaborator

@himdel himdel Mar 23, 2021

Choose a reason for hiding this comment

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

would flex-shrink: 0 help? Sounds like this is caused by flexbox treating width as the optimum, not a constraint.

(that said, this is just moving code from elsewhere so maybe that's not quite related to this PR)

);
}

private updateResources(data) {
console.log('Data: ' + data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

console log (and the comment on line 73?)

@@ -0,0 +1,50 @@
import * as React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

editror -> editor (in the filename)

@@ -53,3 +53,4 @@ export { EmptyStateFilter } from './empty-state/empty-state-filter';
export { EmptyStateUnauthorized } from './empty-state/empty-state-unauthorized';
export { EmptyStateNoData } from './empty-state/empty-state-no-data';
export { EmptyStateCustom } from './empty-state/empty-state-custom';
export { MarkdownEditor } from './markdown-editor/markdown-editror';
Copy link
Collaborator

@himdel himdel Mar 23, 2021

Choose a reason for hiding this comment

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

editror -> editor

this.queryImages(this.state.container.name);
this.queryActivities(this.state.container.name);
this.queryReadme(this.state.container.name);
this.setState({ loading: false });
Copy link
Collaborator

Choose a reason for hiding this comment

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

unlikely to be true at this point?

Copy link
Collaborator

@himdel himdel Mar 24, 2021

Choose a reason for hiding this comment

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

and you seem to have an ordering problem here.. even without this line, the first query* request to complete will reset loading to false, not the last one ... may need 3 separate loading vars, or one method that deals with all the requests and sets state accordingly

(or should only one method be called, depending on the tab?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice :) except now the query* calls from buttons won't unset loading..

<td>{moment(image.pulp_created).fromNow()}</td>
</Tooltip>
<td>{image.layers}</td>
<td>{image.size}</td>
Copy link
Member

Choose a reason for hiding this comment

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

The size returned by the api is in bytes. The value displayed by the UI should be converted to megabytes, rounded to the highest decimal.

Ex:
1,560,000 bytes should display as 1.6 MB
1000 bytes should display as >1 Mb

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be fixed

activities: [],
container: { name: this.props.match.params['container'] },
readme: '',
params: {
Copy link
Member

Choose a reason for hiding this comment

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

params for the search filters, sort and pagination aren't being loaded into the UI when the page loads.

localhost:8002/ui/containers/postgres?id=postgres&tag=latest should populate the filter with the latest tag.

Copy link
Member

Choose a reason for hiding this comment

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

This still isn't working. The parameters are in the URL, but they don't get added to the filter when you load the page.

loading: boolean;
container: { name: string };
readme: string;
images: any[];
Copy link
Member

Choose a reason for hiding this comment

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

Don't use any. Need to add types for the api responses for images, container, and activities.

Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be fixed.

@ZitaNemeckova ZitaNemeckova force-pushed the container_detail_page branch from a8e00f7 to e866cb4 Compare March 26, 2021 11:45
loading: boolean;
container: { name: string };
readme: string;
images: any[];
Copy link
Member

Choose a reason for hiding this comment

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

This still needs to be fixed.

@ZitaNemeckova ZitaNemeckova force-pushed the container_detail_page branch from 6e836e4 to 0bdc151 Compare March 31, 2021 10:16
@ZitaNemeckova ZitaNemeckova force-pushed the container_detail_page branch from a785163 to af1a739 Compare March 31, 2021 15:50
Copy link
Member

@newswangerd newswangerd left a comment

Choose a reason for hiding this comment

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

There are still a few things here that violate best practices, which will make updating and debugging this page later more challenging. However, in the interest of finishing this feature, I'm going to mark this approved and take it on as technical debt for later.

}

render() {
if (this.state.redirect === 'activity') {
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved into the header.

IState
> {
componentDidMount() {
ExecutionEnvironmentAPI.get(this.props.id)
Copy link
Member

Choose a reason for hiding this comment

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

Components should never make API calls unless it's absolutely necessary to do so.


interface IState {
loading: boolean;
container: { name: string };
Copy link
Member

Choose a reason for hiding this comment

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

This should use a container type that gets imported from api/response-types, especially since this type is used on all the container detail views as well as the container list view.

@ZitaNemeckova ZitaNemeckova merged commit 5e49653 into ansible:master Apr 8, 2021
@ZitaNemeckova ZitaNemeckova deleted the container_detail_page branch April 8, 2021 10:04
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

4 participants