[v2] Migrating terra-application-navigation from terra-framework #87
Conversation
tbiethman
requested review from
dkasper-was-taken,
mjhenkes and
ryanthemanuel
as code owners
| import TerraApplicationNavigation from './terra-application-navigation/ApplicationNavigation'; | ||
| import { | ||
| titleConfigPropType, navigationItemsPropType, extensionItemsPropType, utilityItemsPropType, userConfigPropType, | ||
| } from './terra-application-navigation/utils/propTypes'; |
There was a problem hiding this comment.
Should the terra namespace be dropped? Looking at the styles, they are weird seeing terra-application-application
I was also wondering if the structure should be flattened to move the terra-application-navigation contents up a dir level?
src/
application-navigation/
ApplicationNavigation.jsx
index.js
NavigationLayout (instead of terra-application-navigation/ApplicationNavigation)
There was a problem hiding this comment.
I will move things around quite a bit with the introduction of the ApplicationContainer in my next PR. I wanted to get this code landed here first because there's so much of it. A bit of a half-step I know, but I think it'll make more sense to reorganize this in a followup.
I believe we need the --terra-application prefix to align with the naming rules, but I agree that seeing application twice in there is awkward. I can update these again when I come up with a better name for this thing.
| @@ -0,0 +1,84 @@ | |||
| @import '../../breakpoints/media-queries'; | |||
|
|
|||
There was a problem hiding this comment.
Does the terra-application-navigation have themes imports for all of the scss files?
| role: 'listbox', | ||
| }; | ||
|
|
||
| const PopupMenu = ({ |
There was a problem hiding this comment.
I know this is a migration from terra-framework, but I'm wondering if we should add a high level explanation on more complicated packages that have multiple sub-components in it's implementation
There was a problem hiding this comment.
I do believe this would be helpful. I can take a stab at this when I make my changes to the Navigation component to support our new architecture.
| if (isRollup) { | ||
| validatedValue = intl.formatMessage({ id: 'Terra.applicationNavigation.notifications.new' }); | ||
| } else if (value >= 999) { | ||
| validatedValue = '99+'; |
There was a problem hiding this comment.
Should this be 999+ like https://github.com/cerner/terra-application/pull/87/files#diff-31daef00e0f7bfe3c5f4c624e2a8575fR24?
There was a problem hiding this comment.
Hmm this definitely feels off, but I'm not sure whether we should update the check or update the string. @dkasper-was-taken do you remember?
There was a problem hiding this comment.
I think the if check portion is likely what is off. A navigation tab has a width restriction for fitting 99+. I feel like we fixed this elsewhere, but may have missed this one.
There was a problem hiding this comment.
I updated this, and fixing in terra-framework with cerner/terra-framework#1259
| @@ -0,0 +1,156 @@ | |||
| :global { | |||
There was a problem hiding this comment.
Should this be converted or will there be a follow up pr to convert these to be theme imports?
There was a problem hiding this comment.
If there's any modernizing to do for terra-application-navigation, I'd prefer to come back around with a follow-up PR.
…dling across browsers. (#88)
* Added global styles and inserted empty link to the head * Updated changelog * Updated changelog and contributors list * comments * updated application-base to initialize inert * minor change * minor change to imports * Updated changelog
…grate-application-navigation
| height: 100%; | ||
| position: relative; | ||
| top: 0; | ||
| width: var(--terra-application-application-navigation-drawer-default-width, calc(100% - 3.571rem)); |
There was a problem hiding this comment.
i believe this theme variables name be updated later.
Summary
The ApplicationNavigation component is being migrated into terra-application to prepare for its alignment with v2's new architecture. This PR is a port of the previous component package's implementation and jest tests. The only changes include updates to the theme variable names to make them differ from those in the existing package, and the addition of dependencies for the migrated code. Wdio tests were not ported, as they will likely be rewritten as the component changes.