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

Is08 channel mapping cr - filter matrix + rename label feature #53

Merged
merged 53 commits into from
Jan 21, 2021

Conversation

orfar1994
Copy link
Contributor

@orfar1994 orfar1994 commented Jan 13, 2021

This PR include 2 features:

  1. Rename feature - the user can give new name to the output/input/channel.
    image

it will be saved locally. the user can do it by clicking the output/input/channel name (the name text became button when hover it).
image

when the user clicking the name button. the text become editable and the user can edit it.
The user can save it locally be pressing the DoneIcon, cancel editing by pressing ClearIcon (the name will be the last name that saved) and return to the original name by pressing DeleteIcon.

  1. Filter feature - the user can filter the matrix by the following
  • Output label (the real name of the output).
  • Personal Output Label (the new name that the user give to the output).
  • Output channel label (the real name of the output's channel).
  • Personal Output channel Label (the new name that the user give to the output's channel).
  • Input label (the real name of the input).
  • Personal Input Label (the new name that the user give to the Input).
  • Input channel label (the real name of the Input's channel).
  • Personal Input channel Label (the new name that the user give to the Input's channel).
  • Block size (the capability of the input).
  • Reordering (the capability of the input).
  • Routable inputs (the capability of the output).
  • Limit Label Length- the user can cut all the labels in the matrix by this length.
  • Filter Group - the user can add filter group condition (AND or Or), by default it's AND.
    image

@garethsb
Copy link
Contributor

Thanks for opening the PR, @orfar1994. I have had a play with this. The logic of both "personal" names and filtering is basically fine, but I have some concerns about the user interface.

Rename UI

  • I don't like the way the UI resizes as you simply move the mouse over elements (hover, without clicking). At one time I got it into a state where hovering over a name caused it to show the edit button which resized and meant the mouse was no longer over it, so the UI flickered back and forth.
  • I am concerned that the edit fields can be too small when the column is narrow, only two or three characters visible.
  • I am not sure about being able to edit multiple names at the same time
  • I think the presence of ✖️ (cancel) and the bin (delete) icon is a little confusing (delete should be presented at same time as edit)

My suggestion is that the rename functionality should move into the (now interactive) tooltip for the cell. The "personal" name in the tool tip can have edit and delete icons next to it and the edit field can be a sensible consistent width.

(I am also not sure about the word "personal", but this can be easily changed later obviously. Maybe "local", "user", "custom",... Brainstorm required!)

@orfar1994
Copy link
Contributor Author

so the "personal" title (with the edit & delete icon) in the tooltip will be visible always even if we don't have personal name ?

@garethsb
Copy link
Contributor

Following on...

Filtering

  • I think it would be simpler for the user if one filter searched both the "personal" and "original" labels?
  • I'd like to add ID filters as well for consistency with other pages (I've just added ID to the tool tips on master)
  • Can we group Output and Input filters in the drop down, with a divider between? Or have separate Add Input Filters and Add Output Filters?
  • Limiting label length is not a filter, it should be in a separate UI somehow, perhaps under a settings (cog) menu? Or at least separated by a divider for now? (Very minor point but it also needs a minimum of at least one character.)
  • A settings menu would also be a place to add a Delete All Personal Names function?
  • We need to think about the Routable Inputs filter.
    • At the moment if you select a particular Input it shows Outputs that mention that Input but not Outputs that have no routing constraints. If the user is interested in whether they can route a particular Input, those should perhaps be included?
    • Is it searching personal and original names?
    • I wonder if we should make Routable Inputs a Boolean or Const Filter that filters the Outputs based on the current filtered list of Inputs? Although the name seems a bit backwards then, and suggests we ought also have the opposite function of filtering the Inputs based on which can be routed to the current selection of Outputs...

More to come...

@garethsb
Copy link
Contributor

so the "personal" title (with the edit & delete icon) in the tooltip will be visible always even if we don't have personal name ?

I wasn't completely sure, what do you think? Probably needs prototyping to see.

@garethsb
Copy link
Contributor

One issue with using tooltips a lot in this page is how the app performs on touchscreen (tablet). The whole app is already not perfect on small screen of course, but it is something to consider...

@garethsb
Copy link
Contributor

garethsb commented Jan 13, 2021

Rename UI

  • I also remember I was quite confused initially by the fact that the rows automatically re-sort as soon as you ✅ the edit field. It makes me wonder whether alphabetical sorting on label is a bad idea, and instead we want users to be able to move rows and columns manually? Either by drag handles or (on Inputs) up/down buttons that swap the row with the previous/next row... I think the initial sort should be by ID then (or simply unsorted, in the same order as the Device lists the IDs in the response).
  • Of course, editing the label can mean the current filter will now exclude the new value, so we can't avoid that...

@orfar1994
Copy link
Contributor Author

orfar1994 commented Jan 13, 2021

image

image

image

@garethsb
Copy link
Contributor

What do you think? I think that could work well. We can discuss on today's call.

@orfar1994
Copy link
Contributor Author

orfar1994 commented Jan 13, 2021

Rename UI

  • I also remember I was quite confused initially by the fact that the rows automatically re-sort as soon as you ✅ the edit field. It makes me wonder whether alphabetical sorting on label is a bad idea, and instead we want users to be able to move rows and columns manually? Either by drag handles or (on Inputs) up/down buttons that swap the row with the previous/next row... I think the initial sort should be by ID then.
  • Of course, editing the label can mean the current filter will now exclude the new value, so we can't avoid that...

@orfar1994
Copy link
Contributor Author

I agree. it looks better now.
I think the icon should be in the same line of the "Name".
if personal name exists, than the original name title will appear and the personal name will appear under the "name" title.

@garethsb
Copy link
Contributor

garethsb commented Jan 13, 2021

OK, that could work...

I assume the delete (bin) icon would be hidden if there isn't currently a user-provided name?
And that we should add tooltips for channels too of course?

We certainly need to be consistent in how we refer to the two kinds of names/labels.

Suggestions instead of "personal":

  • user name/label (nope, "user name" is bad!)
  • custom name/label
  • local name/label
  • display name/label
  • override name/label (too techy)

Alternatively, if we use "Name" to mean user-provided-if-present-otherwise-underlying-name, (and this ties in with only having a single Filter that searches both) and have a field for the underlying name that appears/disappears depending on whether there is a user-provided name, suggestions for that underlying name field:

  • original name/label
  • device name/label
  • API name/label

None of these are great...

@garethsb
Copy link
Contributor

Also, pressing Return in the edit field should choose the ✔️ so you don't have to click it explicitly to set the new value.

@garethsb
Copy link
Contributor

Hi Or, thanks for all the work on this.

At the moment the key is only based on the inputId/outputId? Did it ought to include the Device ID or something, since inputId and outputId are only unique locally within the Channel Mapping API instance?

Are you OK to resolve this issue?

orfar1994 and others added 5 commits January 13, 2021 20:03
merge sony to master.
…the max length

2. Refactor and use LinkChipField rather than ChipFunctionalLabel (which restores the margin applied by ChipField vs Chip)
2. Replace expressions like (foo ? foo : bar) with idiomatic and more efficient (foo || bar)
3. Remove unnecessary backticks
2. Use 'channel label' rather than 'channel name'
3. Reclaim some horizontal whitespace by using arrow function expressions for the tooltip title functions
@garethsb
Copy link
Contributor

When I have a bit more time, I want to look at refactoring EditableIONameField and EditableChannelLabelField into a single React element. It seems like maybe separate deviceID, ioKey, source, etc. are perhaps not necessary and a single argument that is e.g. set to ${deviceID}.${ioKey}.${source} in the case of Input/Output Name could be passed in.

I've had a go at this over the course of several commits.

…ement that they need to be a single element than can hold a ref.
Reorder parameters to getCustomName/getCustomChannelLabel functions to match hierarchy
…ing of the expanded/collapsed ephemeral setting which was reversed
…d 'Match Any' (or) and tweak some other storage and UI names for better consistency.
There was an issue when e.g. an output name filter and a channel label filter were specified this mode, because filterIOByChannels was called as a second step (and thus always effectively applied with and).
Removing this function will simplify the user experience, make filtering consistent with the List views in the rest of the app, provided by the dataProvider, and simplify code maintenance.
…wing naming style of react-admin useRecordContext, etc.
… when you're tracking along rows or columns of the matrix, using popper.js options
@garethsb garethsb merged commit 9dbdc84 into sony:master Jan 21, 2021
@garethsb
Copy link
Contributor

Thank you, @orfar1994, @rhastie. This is a great addition!

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