feat: add attribute in nested table (PT-186024205) #43
Conversation
emcelroy
force-pushed
the
186024205-add-attribute-in-nested-table-2
branch
from
c9b5466
to
885b797
Compare
There was a problem hiding this comment.
This generally looks good, but I don't understand the focusSet context.
Also when I play around with it, sometimes I can click on an attribute name in the header to edit it, but not always. Perhaps that is related to this focusSet code?
I also found some rendering issues in the Landscape view. This seems to happen when there are 3 levels. But perhaps that is a known issue.
| { | ||
| <button className={css.dropdownIcon} onClick={handleShowHeaderMenu}> | ||
| {showDropdownIcon && <DropdownIcon className={css.dropdownIcon} />} | ||
| </button> | ||
| } |
There was a problem hiding this comment.
The { ... } seems unnecessary here but maybe I'm missing something.
There was a problem hiding this comment.
You're right -- not necessary. I'll remove.
src/components/portrait-view.tsx
Outdated
| const focusSetForLevels = new Map<number, boolean>(); | ||
|
|
||
| const updateFocusSetForLevel = (level: number) => { | ||
| focusSetForLevels.set(level, true); | ||
| }; |
There was a problem hiding this comment.
Can you explain the purpose of this focus set code?
From what I can tell:
- on each render of the portrait view table the focusSetForLevels is reset to a new map
- when updateFocusSetForLevel is called it "sets the focus" for that level.
- when the PortraitViewRow is rendered it "sets the focus" for its parent level if it isn't already set.
- the
shouldGetFocusOnNewAttributeis based on whether the parent of the row focus
Is the idea to try to track whether the a editable header is being edited (so it has focus)?
There was a problem hiding this comment.
@scytacki The idea is to determine which instance of a new attribute's column header should get focus when a new attribute is added.
There can be multiple tables for a single level in the hierarchy. In that situation, when a new attribute is added to that level, only the first table's column header for that attribute should become editable. If we don't track that with the focusSetForLevels map and shouldGetFocusOnNewAttribute, the last/bottom table's column header for the new attribute will get focus instead.
I know this isn't ideal. If you have any thoughts on how it might be better handled given that added context, please let me know. If not, I can at least add a comment explaining the purpose.
There was a problem hiding this comment.
This is what I see when I try it in CODAPv3:
If I understand your intention right, this doesn't seem to be working. I think the reason is because the code assumes the nested tables will be rendered from top to bottom so the first one rendered will get the shouldGetFocusOnNewAttribute. I'm pretty sure the order of the rendering will be random. So this approach won't work. I can't think of a solution off the top of my head, but maybe if we pair we can come up with something. Or maybe this isn't really that important to get right.
#186024205
Adds the ability to add attributes in the nested table view. An "Add Attribute to Collection" button now appears next to the header of each hierarchical level of the table. Clicking the button adds a new attribute, which in turn causes a new column to be added in the target collection's table. The new column is added at the end of the set of table columns, and its header is automatically made editable so the user can change the attribute name from the default
newAttr[n]value.These minor changes are also included:
paddingandshowHeadersoptions are on by defaultresizeCounteras it didn't appear to do what it was intended to do and was interfering with the ability to edit attribute names