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

Change plugin manager styles #2405

Merged
merged 7 commits into from
Nov 5, 2019

Conversation

alexchorman
Copy link
Contributor

No description provided.

@LianaHus LianaHus requested a review from ryestew October 22, 2019 08:45
Comment on lines 49 to +55
.versionWarning {
background-color: var(--light);
padding: 0 7px;
font-weight: bolder;
margin-top: 5px;
text-transform: lowercase;
padding: 4px;
margin: 0 8px;
font-weight: 700;
font-size: 9px;
line-height: 12px;
text-transform: uppercase;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be better as a badge? - I realize that it is currently coded with .versionWarning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it as .badge-light. Check it please

Copy link
Collaborator

@ryestew ryestew Oct 30, 2019

Choose a reason for hiding this comment

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

Sorry - I don't see any .badge-light on plugin-manager-component.js
Please point me to where you changed it. oh - I see it in the side-panel.

Also I think there will be some design adjustment to this page - I'll be reviewing that with Natalia on Thursday.

@@ -13,7 +13,6 @@ const css = csjs`
}
.swapitTitle {
margin: 0;
text-transform: uppercase;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets discuss with Natalia about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Natalia doesn't mind if there are uppercase letters so I made uppercased it again

@@ -122,11 +122,11 @@ export class SidePanel extends AbstractPanel {
name = profile.displayName ? profile.displayName : profile.name
docLink = profile.documentation ? yo`<a href="${profile.documentation}" class="${css.titleInfo}" title="link to documentation" target="_blank"><i aria-hidden="true" class="fas fa-book"></i></a>` : ''
if (profile.version && profile.version.match(/\b(\w*alpha\w*)\b/g)) {
versionWarning = yo`<small title="Version Alpha" class="${css.versionWarning}">alpha</small>`
versionWarning = yo`<small title="Version Alpha" class="badge-light ${css.versionBadge}">alpha</small>`
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryestew I added badge here

@@ -38,7 +38,7 @@ const css = csjs`
.titleInfo {
padding-left: 10px;
}
.versionWarning {
.versionBadge {
background-color: var(--light);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryestew And changed class from .versionWarning to .versionBadge

</div>
<p class="${css.description}">${api.profile.description}</p>
</article>
<article id="remixPluginManagerListItem_${name}" class="list-group-item px-2 pt-2 pb-0 plugins-list-group-item" title="${displayName}" >
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need an indent here. a tab size

Copy link
Contributor Author

@alexchorman alexchorman Nov 4, 2019

Choose a reason for hiding this comment

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

@LianaHus Click "view file" please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you see narrowed version

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry but we need a space.

return yo`
 blablaba
 blablabla
`

Copy link
Collaborator

@LianaHus LianaHus Nov 4, 2019

Choose a reason for hiding this comment

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

what you have is

return yo`
blablaba
blablabla
`

@LianaHus
Copy link
Collaborator

LianaHus commented Nov 4, 2019

I see some problems with position of "settings" button and "active/inactive modul" (why not plugins btw?) in different themes. here are screenshots

q1
q2
q3

z1
z2
z3

@LianaHus
Copy link
Collaborator

LianaHus commented Nov 4, 2019

also why the footer has opacity in dark theme? looks weird and broken to me.
@ryestew

@alexchorman
Copy link
Contributor Author

also why the footer has opacity in dark theme? looks weird and broken to me.
@ryestew

It's not broken. I made according to design.

@alexchorman
Copy link
Contributor Author

I see some problems with position of "settings" button and "active/inactive modul" (why not plugins btw?) in different themes. here are screenshots

q1
q2
q3

z1
z2
z3

Ok, I see. It should be on the same line. Will fix it

@alexchorman
Copy link
Contributor Author

Thanks for the screenshots.

@alexchorman
Copy link
Contributor Author

@LianaHus By "active/inactive module" position problem you mean spaces from left and right sides, don't you? I made those spaces also according to design

@LianaHus
Copy link
Collaborator

LianaHus commented Nov 4, 2019

about opacity I know, this is why I tagged Rob too. But Why we have opacity only on dark theme?

@LianaHus
Copy link
Collaborator

LianaHus commented Nov 4, 2019

yes, the spaces. I don't mind to have spaces for active/inactive modules" but it is different in different themes?

<nav class="navbar navbar-expand-lg navbar-light bg-light justify-content-between align-items-center">
<span class="navbar-brand">Inactive Modules</span>
<span class="badge badge-pill badge-primary" style = "cursor: default;">${inactives.length}</span>
<nav class="plugins-list-header justify-content-start navbar navbar-expand-lg bg-light navbar-light align-items-center px-4 py-3">
Copy link
Collaborator

Choose a reason for hiding this comment

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

px-4 you added here is braking other themes, can't you fix this in nav classes definitions in a theme directly?

@ryestew
Copy link
Collaborator

ryestew commented Nov 4, 2019

The PR is nearly there and we'd like to merge it soon- these issues are small - so just to summarize:

  • the indent on line 122 of src/app/components/plugin-manager-component.js
  • for the footer of the plugin manager: I think the definition of the class remix-bg-opacity should be defined in the csjs and not in the theme's css.
  • the active/inactive module badge - should be the same color (so not badge-alert) & shape ( so remove badge-pill) as the figma
  • the active /inactive modules headline ( the h6) are now aligned with the text below - which is correct - but this has moved these headlines in the other themes - ideally you'd make it so that the change only happens in the EthWorks theme.

I've put these issues in the trello board too.

@@ -126,8 +130,8 @@ export class PluginManagerSettings {

render () {
return yo`
<footer class="navbar navbar-light bg-light ${css.permissions}">
<button onclick="${() => this.openDialog()}" class="btn btn-info">Settings</button>
<footer class="bg-light ${css.permissions} remix-bg-opacity">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this definition of remix-bg-opacity should be defined in the csjs of this file and not in the bootstrap theme. Let me know if you think there is an advantage to leaving it in the theme's css.

@alexchorman
Copy link
Contributor Author

I

The PR is nearly there and we'd like to merge it soon- these issues are small - so just to summarize:

  • the indent on line 122 of src/app/components/plugin-manager-component.js
  • for the footer of the plugin manager: I think the definition of the class remix-bg-opacity should be defined in the csjs and not in the theme's css.
  • the active/inactive module badge - should be the same color (so not badge-alert) & shape ( so remove badge-pill) as the figma
  • the active /inactive modules headline ( the h6) are now aligned with the text below - which is correct - but this has moved these headlines in the other themes - ideally you'd make it so that the change only happens in the EthWorks theme.

I've put these issues in the trello board too.

I think we should leave remix-bg-opacity class because this change should only happen in Ethworks theme

@LianaHus
Copy link
Collaborator

LianaHus commented Nov 5, 2019

thanks for updates!

The last thing is the settings button is still shifted in old themes

q2

@alexchorman
Copy link
Contributor Author

thanks for updates!

The last thing is the settings button is still shifted in old themes

q2

@LianaHus Done.

@LianaHus
Copy link
Collaborator

LianaHus commented Nov 5, 2019

thanks for updates!
The last thing is the settings button is still shifted in old themes

q2

@LianaHus Done.

Cool! When do we merge @ryestew ?

@ryestew ryestew merged commit 81cbca1 into ethereum:master Nov 5, 2019
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.

3 participants