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

UI: Clean up Dynamic UI for CRUD #6994

Merged
merged 20 commits into from
Jul 1, 2019
Merged

UI: Clean up Dynamic UI for CRUD #6994

merged 20 commits into from
Jul 1, 2019

Conversation

madalynrose
Copy link
Contributor

address outstanding comments from #6702, general code cleanup

@madalynrose madalynrose requested a review from a team June 27, 2019 21:55
create: [],
delete: [],
navPaths: [],
});
Copy link
Contributor

Choose a reason for hiding this comment

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

nice cleanup!

</p.top>
<p.levelLeft>
<h1 class="title is-3">
{{capitalize method}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I asked about this before - but do we want the method here or the id - and I don’t think we want to capitalize it - I think ldap is better than Ldap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

method is the id here. naming is weird. I can take out the capitalization though, I agree. I just based it on how we handle this everywhere else

{{#link-to
"vault.cluster.access.method.item.create"
(pluralize itemType)
tagName="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the button here? Need to look and see if it’s a button elsewhere, but I think it should be an anchor.

View {{itemType}}
{{/link-to}}
</li>
<li class="action">
Copy link
Contributor

Choose a reason for hiding this comment

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

canEdit coming soon! :D

{{#if value}}
<code class="is-word-break has-text-black" data-test-row-value="{{label}}">{{value}}</code>
{{else}}
{{#if (eq valueType "array")}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to show [] or {} if there’s no value for it?

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 looks really weird if it’s just nothing imo. @joshuaogle what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe No value assigned? Or even Default value ? (but I’m not sure if there’s no value if it is the default value…). I think we should check for ttl types though - they’re currently showing a {} and I think we don’t want that.

What do you think about leaving these out for now and we can get them in after we talk through the various states - I agree that the blank is strange. Should we default config pages to hide items that are empty maybe?

Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

This looks good! I had a couple of questions, but I think there were two other things one that came up when I showing UI stuff to @joshuaogle and I didn’t get a chance to circle back to yet.

1 - can we make the link to LDAP from the auth page have black text instead of grey like the others (to show you can click on it)
2 - does this address the routing issue where you click on LDAP in the list and you land on the configuration page? I think this should go to the groups page (or whichever is the first tab).

meirish
meirish previously approved these changes Jun 28, 2019
Copy link
Contributor

@meirish meirish left a comment

Choose a reason for hiding this comment

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

Nice - thanks for circling back and getting all of these! Approving, but there’s an open question on what to do with the info-table-row component. I think we should decide on what to do there before merging.

@madalynrose madalynrose merged commit 15d2fdd into master Jul 1, 2019
@madalynrose madalynrose deleted the ui-gen-crud-cleanup branch July 1, 2019 20:36
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