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.

Sign up for GitHub

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

Jump to bottom

New personabar Web Servers tab #4408

Merged
merged 14 commits into from
Jan 20, 2021
Merged

New personabar Web Servers tab #4408

merged 14 commits into from
Jan 20, 2021

Conversation

donker
Copy link
Contributor

@donker donker commented Jan 9, 2021

Although the platform keeps track of web servers, there is no way to manage this data. This PR brings a web servers tab to the "Servers" personabar module as follows:

Screenshot 2021-01-09 at 22 47 20

You're able to edit the url for a web server and to delete them.

@donker donker added this to the 9.9.0 milestone Jan 9, 2021
@mitchelsellers
Copy link
Contributor

@donker thank you so much for the work on this, seeing this I'm curious if we can make a few small adjustments.

  1. Can we get the time of last activity displayed as well? In situations where a server is brought online mid-day the last activity time will be critical.
  2. Can we hide the "Remove inactive" if there are no servers in that is.
  3. Can we add a server icon to the beginning of the row, and have it solid colored if active & greyed our if inactive? (Thinking similar style to how we do things in the "Connectors" section

Items 2 & 3 are really nice to haves if at all possible.

@donker
Copy link
Contributor Author

donker commented Jan 10, 2021

@donker thank you so much for the work on this, seeing this I'm curious if we can make a few small adjustments.

1. Can we get the time of last activity displayed as well?  In situations where a server is brought online mid-day the last activity time will be critical.

2. Can we hide the "Remove inactive" if there are no servers in that is.

3. Can we add a server icon to the beginning of the row, and have it solid colored if active & greyed our if inactive?  (Thinking similar style to how we do things in the "Connectors" section

Items 2 & 3 are really nice to haves if at all possible.

Check, check, and check

@WillStrohl
Copy link
Contributor

🥇

mitchelsellers
mitchelsellers previously requested changes Jan 11, 2021
View reviewed changes
Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

Overall this looks good, worked well with a big list of servers as well. I did have two suggested changes for the help text, but still not 100% sure on the verbiage.

I would like @valadas or one of the others to review the React.Common changes for clarity.

...ience/Dnn.PersonaBar.Extensions/admin/personaBar/Dnn.Servers/App_LocalResources/Servers.resx Outdated Show resolved Hide resolved
...ience/Dnn.PersonaBar.Extensions/admin/personaBar/Dnn.Servers/App_LocalResources/Servers.resx Outdated Show resolved Hide resolved
donker and others added 2 commits January 11, 2021 10:05
@donker @mitchelsellers
Update Dnn.AdminExperience/Dnn.PersonaBar.Extensions/admin/personaBar…
57598de 
…/Dnn.Servers/App_LocalResources/Servers.resx

Co-authored-by: Mitchel Sellers <msellers@Iowacomputergurus.com>
@donker @mitchelsellers
Update Dnn.AdminExperience/Dnn.PersonaBar.Extensions/admin/personaBar…
f62b890 
…/Dnn.Servers/App_LocalResources/Servers.resx

Co-authored-by: Mitchel Sellers <msellers@Iowacomputergurus.com>
valadas
valadas approved these changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

I see nothing wrong in code and downloaded the CI build and it looks fine to me.
Thanks @donker

mtrutledge
mtrutledge approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@mtrutledge mtrutledge 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. There is one spot where we reference a custom icon, but the icon exists in the SvgIcons we distribute. A new type should be added to the getIcon function of the IconButton Component. This would reserve the CustomIcon prop to components that use SVG icons that are not distributed with the Common library.

Dnn.AdminExperience/ClientSide/Roles.Web/src/components/roles/UsersInRole/UserRow/index.jsx
@@ -110,7 +109,7 @@ class UserRow extends Component {
showIcon={true} showInput={false}
onIconClick={this.onExpiresTimeClick.bind(this, props.userDetails, props.index) } />
</span> : null;
let deleteAction = props.userDetails.allowDelete ? <IconButton type="x" width={17} onClick={this.onDeleteClick.bind(this, props.userDetails, props.index) } /> : null;
let deleteAction = props.userDetails.allowDelete ? <IconButton customIcon={require("!raw-loader!../../../../img/common/x.svg").default} width={17} onClick={this.onDeleteClick.bind(this, props.userDetails, props.index) } /> : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point we should add a type for all the pre-packaged svg icons we include and leave the custom icon for icons that are specific to individual components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree. We can do this in a separate PR for 9.9 as well if one has time.

@mtrutledge mtrutledge merged commit 13caec2 into dnnsoftware:develop Jan 20, 2021
@donker donker deleted the web-servers-tab branch January 21, 2021 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrutledge mtrutledge mtrutledge approved these changes

@valadas valadas valadas approved these changes

@bdukes bdukes Awaiting requested review from bdukes

@mitchelsellers mitchelsellers Awaiting requested review from mitchelsellers

Assignees
No one assigned
Labels
Area: AE > PersonaBar Ext > Servers.Web Type: Feature
Projects
None yet
Milestone
9.9.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@donker @mitchelsellers @WillStrohl @mtrutledge @valadas