-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Deployed locally and everything seems to work. Really like the work done with the radio buttons for using a range revision rule vs a specific revision, too.
Let me know if you'd like to spend some time pairing on the issues you listed in the OP. I'm free most this week (asides Wednesday/Thursday - I'll be OoO on those days).
<div class="columns"> | ||
<div class="column"> | ||
<ul class="rev-list"> | ||
@foreach (var revision in Model.Revisions.OrderByDescending(r => r.OrderKey())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding a TODO note to tie in #134 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you have here is more like the "revisions" table in the existing UI, which tells the user what revisions are registered and where they are active. I would definitely not describe this as "recent activity" - e.g. there might be recent changes to a legacy deployment whose version number might be quite old. Correspondingly, I would describe this as "is currently active on" rather than "was pushed to" - we are not observing the push event here, only the current deployment state.
We can choose whether to have the 'all revisions' list, the 'recent activity' feed or both - I'm just saying we need to label them appropriately.
(Another idea for the revisions list, if we want one, would be to have an 'active revisions' list on the main app page but keep the 'all revisions' list off on a linked page or behind a 'see all' toggle button.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @itowlson I am going to rework this section (and the terminology therein) in a follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks amazing. One minor comment on wording where we are conflating two different things. Otherwise eager to get this in as soon as you've addressed the bits you said are currently not working.
<div class="columns"> | ||
<div class="column"> | ||
<ul class="rev-list"> | ||
@foreach (var revision in Model.Revisions.OrderByDescending(r => r.OrderKey())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you have here is more like the "revisions" table in the existing UI, which tells the user what revisions are registered and where they are active. I would definitely not describe this as "recent activity" - e.g. there might be recent changes to a legacy deployment whose version number might be quite old. Correspondingly, I would describe this as "is currently active on" rather than "was pushed to" - we are not observing the push event here, only the current deployment state.
We can choose whether to have the 'all revisions' list, the 'recent activity' feed or both - I'm just saying we need to label them appropriately.
(Another idea for the revisions list, if we want one, would be to have an 'active revisions' list on the main app page but keep the 'all revisions' list off on a linked page or behind a 'see all' toggle button.)
Would you mind rebasing when you get the chance? Thanks! |
Signed-off-by: flynnduism <dev@ronan.design>
Signed-off-by: flynnduism <dev@ronan.design>
* fixes directory casing mismatch issue (Hippo > hippo) * includes fix for the App/EditChannel and App/NewChannel forms * fixes controller issue for App/New Signed-off-by: flynnduism <dev@ronan.design>
Signed-off-by: flynnduism <dev@ronan.design>
Signed-off-by: flynnduism <dev@ronan.design>
64d81d0
to
41068ba
Compare
Signed-off-by: flynnduism <dev@ronan.design>
Signed-off-by: flynnduism <dev@ronan.design>
|
||
<div class="tabs"> | ||
<ul> | ||
<!-- an initial/default environment, auto-generate when creating an app --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@Html.DisplayFor(modelItem => item.Name) | ||
</p> | ||
</a> | ||
<a class="env-url" target="_blank"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this clickable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's now clickable
I've been putting this PR through its paces today, and it looks like it's ready to go! |
Signed-off-by: flynnduism <dev@ronan.design>
Signed-off-by: flynnduism <dev@ronan.design>
This PR updates the UI with new layouts and styles. This replaces #105.
Main changes:
Views updated:
Missing Pieces
There are some additional parts of this that aren't working properly, and I will be seeking help to 🍐 get them functional:
App/Edit routing is breakingApp/EditChannel form isn't submitting correctlyApp/NewChannel form isn't submitting correctlySome Screenshots