Skip to content

🚸 51 - project id error#62

Merged
anncatton merged 3 commits into
developfrom
51-project-id-error
Mar 3, 2021
Merged

🚸 51 - project id error#62
anncatton merged 3 commits into
developfrom
51-project-id-error

Conversation

@anncatton

@anncatton anncatton commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

Adds error messages for various configuration errors related to Arranger.
Adds styling and icons

);
};

const WarningListItem = ({ fieldName }: { fieldName: string }) => (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is duplication of styling here between WarningListItem and ListItem

Might be worth creating one component for ListItem and then having a Missing type set of props.
Or even a MissingListItem component but use a common List component.
eg.

const MissingListItem = () => <ListItem fieldName=''..." value=''Missing" icon={<Warning/>}/>

Comment thread components/pages/repository/index.tsx Outdated

const [availableProjects, setAvailableProjects] = useState<Project[]>([]);
useEffect(() => {
fetch(`https://arranger.qa.overture.bio/admin/graphql`, {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

url root best in an .env variable, make it easy to configure

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh whoops i have this env var already, i forgot to use it!

);
};

export default Error;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just out of interest, how do you like doing icons like this? We do it this way on ARGO and it was a little annoying to start with

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like it - I think because i got used to it on argo. It feels like dealing with icons is a lot more straightforward this way because they're just like any other component

@ciaranschutte
ciaranschutte self-requested a review March 2, 2021 15:41
@ciaranschutte

Copy link
Copy Markdown
Contributor

@anncatton left some comments, let me know what you think

@anncatton
anncatton merged commit b6fa8f3 into develop Mar 3, 2021
@anncatton
anncatton deleted the 51-project-id-error branch March 3, 2021 15:51
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.

2 participants