CSCFC4EMSCR-665 pull request to develop #245
Conversation
…moved commented out code, changed visibility and position of Create Mapping button, creted separated tabs for Crosswalk view and Schema view, added scrollbar to center column of Crosswalk editor and maximum height for center column, added tooltip for blue balls indicating use of schema tree node on Crosswalk mappings.
…ld version of schema-info.
| @@ -62,6 +66,8 @@ export default function SchemaInfo(props: { | |||
| const [showAttributeNames, setShowAttributeNames] = useState(true); | |||
| const [treeDataOriginal, setTreeDataOriginal] = useState<RenderTree[]>([]); | |||
|
|
|||
| const [modalOpen, setModalOpen] = useState(false); | |||
There was a problem hiding this comment.
Is this modal going to be the mapping read-only view or what? For now I couldn't find anything this controls except for the click listener.
| <CircleIcon string={'Schema tree node is used on Crosswalk mappinng.'} style={{color: "#1976d2", maxHeight: "25%", maxWidth: "25%"}}/> | ||
| </Tooltip>; | ||
| } else { | ||
| return <div></div>; |
There was a problem hiding this comment.
Would it be possible to return a fragment <></> instead? They get cleaned up so wouldn't leave an empty div around.
There was a problem hiding this comment.
Maybe. Never tried to return <></> instead of div.
There was a problem hiding this comment.
Instead of splitting the tab menus to two different components, couldn't you just have made the mappings tab only applicable for crosswalk?
{!isRemoved && contentType == Type.Crosswalk && (
<StyledTab {...customTabProps('mapping-accordion-tab')} />
)}
Need to add a translation for the key SCHEMA.mapping-accordion-tab as well, but not much more modification is needed I think.
There was a problem hiding this comment.
This was another bug fix requested by @rquazi. Schema view had empty tab because tab component was used on both schema view and crosswalk view. I made another component, because there were two set of tabs. I may not figure out your suggestion. Is it going to hide tab based on contentType?
There was a problem hiding this comment.
Yes, it would then only show that tab option when you're in the crosswalk editor.
There was a problem hiding this comment.
I do think the tab menu should be kept as a single component, the variation between views is so minor. Essentially we want the tab menu to look identical in both schema view and crosswalk view, just one more tab in the crosswalk view. I leave it to @masillan to decide whether to revert back to single component in this branch and ticket or whether a new ticket should be made.
There was a problem hiding this comment.
I think that tab menus should not be shared at all, because they are completely different and just by chance they have had same tabulator names. Sharing them can cause errors, which have already happened. When project does not have any tests that could find those errors automatically sharing tabulator makes even less sense.
There was a problem hiding this comment.
Let's make it a separate ticket then, and refer back to this MR to help with implementing that when/if we have time and it's sensible. Here's my reasoning for why it would be sensible:
The contents of the tab panels were already separate, they are given to the tab selector component as the parameter TabPanels, each specifying what the tab should show. The index of the tab combined with the global constant MscrTabs identifies the tabs and guides what the text in the tab selector should be. The Tabmenu component has the code patterns for customizing what to show in the menu, which is why it can be shared between them. I know it's not the easiest component to read and understand, but once you figure out the mechanics, you can make it render any kind of tab menu you like, demonstrated by for example by the stub menu for deleted content.
It is not by chance some of the tabs had the same names, the text to show for each tab is specified by the customTabProps by converting it to a i18next call with the translations dict. The shared tab names are linked to the same translation key, eg.
'SCHEMA.history-tab': t('tabs.history-tab'),
'CROSSWALK.history-tab': t('tabs.history-tab')
But they don't have to be the same. The inconvenience with the current implementation of the translation dict is that it implies you'll have to make a translation entry for a new kind of tab for both SCHEMA and CROSSWALK, even if it's only ever visible in crosswalk, eg,
'SCHEMA.mapping-tab': t('tabs.mapping-tab'),
'CROSSWALK.mapping-tab': t('tabs.mapping-tab')
There are ways to get around that with extra conditionals in other places, if that is an issue.
It is true that the lack of automated testing means we're always in danger of regressing. That means keeping code easily maintainable is even more important. Having long strings of duplicate code where you'll have to copy back and forth any changes you want to be reflected on both schemas and crosswalks (we do want to keep the layout consistent) might not decrease the chance of errors.
…and replaced div with <> element. Changed size of components to fit them better on screen. Starting crosswalk editor with no mappings displayed.
CSCFC4EMSCR-665 Implement new CW layout: Moved schema node info to pop up window.