Create new component to select archive type #395
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meisekimiu
force-pushed
the
glam-select-archive-type
branch
from
a0f68bf to
7eab528
Compare
crisnicandrei
approved these changes
May 21, 2024
|
Personally, I don't really like the border that appears when you click an archive type, but that is subjective. Besides that, this looks great! |
|
I'm going to remove some of the unnecessary comments from the CSS, and I will also add the |
The current DialogService used in the web-app uses deprecated Angular functionality and a few other architectural issues. The new archive type selection component uses a modal to select a type, so this is a good opportunity to finally start migrating over to using the Angular CDK Dialog instead of our custom DialogService. Create a new service that wraps around Angular CDK's DialogService. The reason to do this right now is so that there is little confusion over which `DialogService` components are using, though in the future it would probably be best to remove this wrapper service and import the Angular CDK DialogService once we have completely deleted the old one. See #269 for more information. PER-9568: Create new component to select archive type
Create the pr-glam-archive-type-select component, which will be used to select the archive type in a future redesigned onboarding flow. This also includes the archive-type-select-dialog component, which is the component shown in a modal that is actually what does the selecting. This commit includes the basic component functionality made through test-driven development with no styles or accessibility features. These will come in future commits. PER-9568: Create new component to select archive type
This CSS is needed to properly display the overlay shown in the Angular CDK Dialog component. We may want to make our own dialog component later, but for now let's use the default CSS. PER-9568: Create new component to select archive type
Set up some basic styles for the ArchiveTypeSelectDialogComponent. This also includes installing the regular FontAwesome icon pack in addition to the solid icons. PER-9568: Create new component to select archive type
We want to use icons to represent archive type in a few places throughout the onboarding process. Instead of hardcoding it in each component, extract this logic into a single component that acts as a wrapper for the fa-icon component. PER-9568: Create new component to select archive type
Style the glam-archive-type-select component to match designs. Also utilize the newly extracted archive-type-icon component. PER-9568: Create new component to select archive type
This is a required HTML attribute; add it here for validity and accessibilty purposes. PER-9568: Create new component to select archive type
These were left over from a figma copy/paste. Get rid of them. PER-9568: Create new component to select archive type
meisekimiu
force-pushed
the
glam-select-archive-type
branch
from
ca681be to
cd73e42
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds some new components to be used in the newly redesigned "GLAM" onboarding flow. These components are currently unused since integrating them will be done later, but the components can be tested with Storybook.
One pretty significant change here is that the new archive type selection component uses the Angular CDK Dialog service instead of our custom-rolled
DialogService. As discussed in #269, the currentDialogServiceis a bit problematic and we want to eventually move away from it. One big problem with it I noticed while working on this ticket is that it violates some design principles (the Open Closed Principle and probably others) as the DialogService needs to know about the names of possible components that it can host (see this type definition) and also needs module-level configuration to work properly.I did not want to continue going along with this design for these glam components, especially since these components are probably more volatile than normal (they may be moved around the codebase, they currently exist as standalone components, they may be renamed to remove "glam" from their name in the future). So I decided to use the CDK DialogService for these components instead. To do so, I ended up creating a wrapper service so that we could potentially end up using a form of branch by abstraction to migrate older code over. Also I wanted it to be clear when components were using the new CDK DialogService. In the future when we migrate over fully, we shouldn't really need the wrapper service anymore and we should import the
AngularCdkDialogModuledirectly instead.Steps to test:
npm run storybookIncomplete design questions: