Edit rounds ui! #1856
Edit rounds ui! #1856
Conversation
jfly
added
the
STATUS: in-progress
|
Can't see the google doc link. |
|
Oops! Updated the original post. |
jfly
force-pushed
the
edit-rounds-ui
branch
4 times, most recently
from
5513dc7
to
5872ff4
Compare
jfly
removed
the
STATUS: in-progress
|
I'm initially concerned with the selection of saying if it's being held / how many rounds. With it being grayed out, I expect not to be able to interact with it even though I'm supposed to interact with it. Maybe for fun, not being able to select certain cutoffs when certain round formats are selected.
|
|
Thanks for the feedback, @coder13! I've fixed up the advancement condition UI. I've also added a feature spec of editing events/rounds, which required adding an annoying amount of polyfill junk just to make PhantomJS happy. |
jfly
force-pushed
the
edit-rounds-ui
branch
2 times, most recently
from
74155f3
to
14b3627
Compare
|
Sorry for the delay, here is my feedback:
|
jfly
force-pushed
the
edit-rounds-ui
branch
2 times, most recently
from
c02c0a3
to
074c297
Compare
|
Thanks for the feedback, @Laura-O! Some responses:
Ah, you must be at an unfortunate screen size, then. If I make this look good for you (2 columns of events instead of 3), it will be wasted space for someone with my screen size. With some work, we can probably find something that's good for both of us, but for now, I don't think it's worth the effort right now. Let's address this later in #1916. Just to be clear, the page is still usable for you, right? There should be horizontal scrollbars when stuff doesn't fit, but I don't see them in your screenshot, which I'm hoping is because you're on a mac and they hide scrollbars, not because the scrollbars don't exist.
I like this idea! I've gone ahead and made this change in jfly@03def6c. I figured it would be more useful to just put the button in the alert, how does this look?
Sorry, I'm not sure what you mean here. Maybe it would be best to chat in real time about this?
So I tried setting the opacity of events that are not being held to 50% to try to reduce visual clutter, but I guess that's not good enough =(
Haha, that's not merely something we allow delegate to do, we require them to do it! Currently competitions are required to have at least 1 event before being confirmed.
The new UI you see is an update of the current /events/edit page, so that page you're thinking of does not exist anymore. Just to be clear, here's what I currently see on https://www.worldcubeassociation.org/competitions/BerkeleyWinter2017/events/edit:
When we merge this PR, the above page will change to have all the event panels you've been playing around with.
Done in jfly@ab57c0b.
This is a little tricky to implement, and I didn't build the infrastructure for validations yet. I'm going to suggest that we address this in #1916.
The reset button resets the modal back to the state it was in at the time you opened the modal dialog. It sounds like you expect it to reset the modal back to the state the WCIF was in when you first visited the page? I personally think the current behavior is more useful, because if you want to reset the WCIF back to the state it was in when you first visited the page, you can refresh the page. However if this is just too confusing, maybe we should get rid of the reset buttons entirely?
Done in jfly@f67cf2c. How does this look?
Ah, good point! Fixed in jfly@abcd254. |
|
Updated in response to review comments. I pushed some non-critical suggested out into #1916, which I'll address in a later PR. For now, I believe this is ready to go, and I hope to merge this up on Monday (but I'd really love some people to take a look at the code first!) |
|
Code looks good to me. 👍 |
|
Skipping some comments as I was confused and @jfly already enlightened me when we chatted about this. 😄
Yes, the screenshot was on a 13" MBP. It's still usable.
Yes, maybe removing is the best solution.
Looks good! 👍 |
(It's very possible I'm misunderstanding you here, so please tell me if I am.) I don't think it's fair to describe this as "not allowing resets". It seems to me that we just have different definitions of "reset". My definition is "reset takes you back to the state the modal was in when you opened the modal". Your definition is "reset takes you back to the state this was in when you first loaded the page". I'd like to get some other people's opinions on what they think makes sense here. |
|
Reset should reset to when I first started interacted with whatever thing I am trying to reset. |
|
It works exactly as I expect it to, does something that I want but I can see where Laura is coming from and maybe it's not the most properly named? Sometimes I do some changes and want to go back to where it was. I might close out of the dialog and reopen it, or hit the reset button. |
|
I had a chat with @FatBoyXPC and @coder13 and we agreed that the behavior of the dialogs was confusing. Here's the system we decided is best:
I've implemented this in dca0f2b, and you can see it live on staging now! |
|
|
||
| function normalizeWcifEvents(wcifEvents) { | ||
| return events.official.map(event => { | ||
| return wcifEvents.find(wcifEvent => wcifEvent.id == event.id) || { id: event.id, rounds: [] }; |
There was a problem hiding this comment.
What about _.find(wcifEvents, { id: event.id }) || ...?
|
|
||
| export default { | ||
| official: <%= Event.official.to_json.html_safe %>.map(extend), | ||
| byId: mapObjectValues(<%= Event.all.index_by(&:id).to_json.html_safe %>, extend), |
There was a problem hiding this comment.
Let's use Lodash neat function =)
|
|
||
| function deepEqual(obj1, obj2) { | ||
| return JSON.stringify(obj1) == JSON.stringify(obj2); | ||
| } |
There was a problem hiding this comment.
I believe Lodash have these functions (cloneDeep and isEqual accordingly).
| this.setState({ savedWcifEvents: clone(this.props.wcifEvents), saving: false }); | ||
| }).catch(e => { | ||
| this.setState({ saving: false }); | ||
| alert("Something went wrong while saving.\n" + e.message); |
There was a problem hiding this comment.
A template string, please =)
| this.setState({ saving: true }); | ||
| promiseSaveWcif(wcif).then(response => { | ||
| return response.json().then(json => [ response, json ]); | ||
| }).then(([response, json]) => { |
There was a problem hiding this comment.
A matter of preference, but I would write this like that:
promiseSaveWcif(wcif)
.then(response => Promise.all([response, response.json()]))
.then(([response, json]) => {
/* ... */
}There was a problem hiding this comment.
I like the use of Promise.all here!
Not sure if you also meant to make a point about indentation here, but I do not really like indenting my chained thens, because it looks weird when the chain ends and the level of indentation is more than 1 level off from the next one. See here for a discussion including some folks who also have my school of thought on this.
I also don't want this PR to devolve into a discussion about javascript code formatting. I would like for us to get a linter and style checker set up, and then we can discuss how we want that thing to be configured. I've created #1924 to continue this discussion.
| }); | ||
| }); | ||
| return roundsSharingTimeLimit; | ||
| } |
There was a problem hiding this comment.
What about this?
function findRoundsSharingTimeLimitWithRound(wcifEvents, wcifRound) {
return _.flatMap(wcifEvents, 'rounds').filter(otherWcifRound =>
otherWcifRound !== wcifRound
&& otherWcifRound.timeLimit
&& otherWcifRound.timeLimit.cumulativeRoundIds.includes(wcifRound.id)
);
}| }); | ||
| }); | ||
| return wcifRounds; | ||
| } |
There was a problem hiding this comment.
What about this?
function findRounds(wcifEvents, roundIds) {
return _.flatMap(wcifEvents, 'rounds').filter(wcifRound => roundIds.include(wcifRound.id));
}| } | ||
|
|
||
| hasUnsavedChanges = () => { | ||
| return JSON.stringify(this.getSavedValue()) != JSON.stringify(this.state.value); |
There was a problem hiding this comment.
Let's Lodash do the trick return _.isEqual(this.getSavedValue(), this.state.value);
There was a problem hiding this comment.
Nit, we want to check if they're not equal, not if if they are equal =)
| } | ||
|
|
||
| onChange = (value) => { | ||
| this.setState({ value: value }); |
There was a problem hiding this comment.
You can simply do this.setState({ value }); as the key name matches the variable name =)
| throw new Error(); | ||
| } | ||
| otherWcifRound.timeLimit.cumulativeRoundIds.splice(index, 1); | ||
| }); |
There was a problem hiding this comment.
I think this could simply be:
findRoundsSharingTimeLimitWithRound(this.props.wcifEvents, wcifRound).forEach(otherWcifRound => {
_.pull(otherWcifRound.timeLimit.cumulativeRoundIds, wcifRound.id);
});
The AttemptResultInput component still needs to be implemented, but I think it provides the right abstraction.
Also formatting our selects consistently.
Also updated the error message dialog to include the error from the server, so the user has some idea what's going on.
…med competitions.
one that I do not believe we can fix.
message before removing existing rounds of an event.
- Spread the rounds across multiple rows instead of columns. - Improved explanation for cutoff and time limit.
A competition event with 0 rounds means "yes, we're holding this event, but the number of rounds has not been decided yet or has not been announced yet"
jfly
force-pushed
the
edit-rounds-ui
branch
from
2810de6
to
f874096
Compare
It seems that the demo competition still has 100% set for the first round of multi. ;-) |
|
Yeah, it's just a silly ui input max right now. Later improvement will be to validate this server side. |
Now that #1619 is finally merged (afer over 4 months =(), we now have a
PATCH /competitions/:id/wcif/eventsendpoint!This PR introduces a React based UI to edit the events and rounds of a competition. @Laura-O was kind enough to help out with the design of the UI components, which you can see in this google doc.
You can try this out on staging!
This PR is meant to be the minimum functionality required to let users input rounds and round attributes. I will address non critical stuff like UI polishing and validations in a later PR. I am tracking ideas for improvement in issue #1916.
TODO