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

storaged: Use the list pattern for content #5097

Closed

Conversation

Projects
None yet
4 participants
@mvollmer
Copy link
Member

commented Sep 29, 2016

This is still pretty rough. The tabs are there, but their contents is very raw.

Be careful. There are some glitches in the way things are updated when rows/tabs are added and removed, and I suspect that sometimes a button doesn't do what it says on the tin. It might just delete the wrong thing.

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

Depends on #4854

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch from c454f33 to 953874b Sep 29, 2016

@mvollmer mvollmer added the bot label Sep 29, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

There are some glitches [...]

Hmm, I had a messed up multipath setup, which might explain most of the weirdness I have seen. (Mounting a volume would cause the whole volume group to disappear without a trace...) Kids, be careful out there with multipath.

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch 11 times, most recently from ec927d8 to 87956f3 Sep 29, 2016

@mvollmer mvollmer changed the title WIP - List pattern for storage storaged: use the list pattern for content Oct 4, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2016

Tests needs adapting.

@mvollmer mvollmer removed the bot label Oct 4, 2016

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch from 87956f3 to b61c339 Oct 4, 2016

@mvollmer mvollmer changed the title storaged: use the list pattern for content storaged: uUse the list pattern for content Oct 4, 2016

@mvollmer mvollmer changed the title storaged: uUse the list pattern for content storaged: Use the list pattern for content Oct 4, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2016

screenshot from 2016-10-04 15-40-10
@andreasn, the buttons need some aligning or other polish. Any ideas?

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch 2 times, most recently from a4a59d7 to 4bd1868 Oct 4, 2016

@dperpeet dperpeet removed the blocked label Oct 5, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

needs a rebase

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch from 4bd1868 to 692ab53 Oct 6, 2016

@stefwalter stefwalter added needswork and removed blocked labels Nov 16, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2016

This needs a rebase

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch 2 times, most recently from 16370a0 to 7f0bc40 Nov 17, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

I have put this on top of #5401.

@mvollmer mvollmer added blocked and removed needswork labels Nov 17, 2016

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch from 7f0bc40 to f397e7f Nov 17, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2016

Holy rebase debacle, batman.

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch 2 times, most recently from f48b52e to 3c7628e Nov 17, 2016

@mvollmer mvollmer removed the blocked label Nov 18, 2016

@dperpeet
Copy link
Member

left a comment

needs some changes, mostly code formatting but also one design issue

</td>
</tr>
{ !self.props.client.is_old_udisks2 ?
<tr>

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

According to the coding guidelines, we want to wrap multiline jsx in parentheses. Now, this is a case of nested jsx...
In any case, I think the indentation needs to change - according to Crockford because of the operator ? and since this is inside { }.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

I agree about the indentation. Fixed.

</tr> : null
}
{ old_opts?
<tr>

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

According to the coding guidelines, we want to wrap multiline jsx in parentheses. Now, this is a case of nested jsx...
In any case, I think the indentation needs to change - according to Crockford because of the operator ? and since this is inside { }.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Fixed.

</tr> : null
}
{ (mounted_at.length > 0 || self.props.client.is_old_udisks2) ?
<tr>

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

According to the coding guidelines, we want to wrap multiline jsx in parentheses. Now, this is a case of nested jsx...
In any case, I think the indentation needs to change - according to Crockford because of the operator ? and since this is inside { }.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Fixed.

<td>
{mounted_at.join(", ")}
{ mounted_at.length > 0 ?
<StorageButton onClick={unmount}>{_("Unmount")}</StorageButton> :

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

According to the coding guidelines, we want to wrap multiline jsx in parentheses. Now, this is a case of nested jsx...
In any case, I think the indentation needs to change - according to Crockford because of the operator ? and since this is inside { }.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Fixed.

<StorageButton onClick={stop}>{_("Stop")}</StorageButton> :
<StorageButton onClick={start}>{_("Start")}</StorageButton>
}
<FormatButton client={this.props.client} block={this.props.block}/>

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

Technically this shouldn't be indented as far, but I agree this makes sense in a way. Would this be simpler if there were a variable storage_button that is defined before return?

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Fixed. This was just the Emacs web-mode screwing up.

text: _("Pool for Thin Volumes")
};
tabs = create_tabs (client, lvol, false);
append_row (rows, level, lvol.Name, lvol.Name, desc, tabs, false);

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

extra whitespace after function name

text: lvol.Active? _("Unsupported volume") : _("Inactive volume")
}
tabs = create_tabs (client, lvol, false);
append_row (rows, level, lvol.Name, lvol.Name, desc, tabs, false);

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

extra whitespace after function name

};
}

function append_row (rows, level, key, name, desc, tabs, job_object) {

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

extra whitespace after function name

var _ = cockpit.gettext;
var C_ = cockpit.gettext;

function create_tabs (client, target, is_partition) {

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

extra whitespace after function name - this is the case in a lot of places in this pull request

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Fixed in the *.jsxfiles.

};
}

function append_row (rows, level, key, name, desc, tabs, job_object) {

This comment has been minimized.

Copy link
@dperpeet

dperpeet Nov 18, 2016

Member

A size of 0 doesn't have a unit, which looks a bit strange. On master, there is no 0. I suggest writing 0 B or empty.

screenshot from 2016-11-18 17-14-12

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

This is a separate issue about cockpit.format_bytes etc, no?

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Well, we should probably not show content for a cd-rom without a disk in it....

This comment has been minimized.

Copy link
@mvollmer

mvollmer Nov 22, 2016

Author Member

Fixed

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2016

Marking priority due to sprint.

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch 2 times, most recently from 2544b04 to 9353e0b Nov 22, 2016

@mvollmer mvollmer removed the needswork label Nov 22, 2016

@mvollmer mvollmer force-pushed the mvollmer:storaged-list-pattern branch from 9353e0b to a3dc134 Nov 22, 2016

stefwalter added a commit that referenced this pull request Nov 22, 2016

storaged: Use the list pattern for content
Closes #5097
Signed-off-by: Stef Walter <stefw@redhat.com>
 * Fixed two whitespace issues found during review.

stefwalter added a commit that referenced this pull request Nov 22, 2016

test: Adapt storage tests to previous change
Closes #5097
Reviewed-by: Stef Walter <stefw@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.