-
Notifications
You must be signed in to change notification settings - Fork 16
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
263 - Add file metadata section to the File page #281
263 - Add file metadata section to the File page #281
Conversation
…into feature/263-add-filemetadata-section-to-the-file-page
During development, I saw that it would be good to move the file metadata into its own model. This way, it can be used in both the File and FilePreview models. This was needed for the 'Access File' button. So, instead of waiting for another sprint, I did it now. I made a different PR for the refactor, which is here: #281 My idea is to look at both PRs one by one. Then, we can put the refactor into this branch for combined QA of both changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this scss file only contains one file, we could similarly add a <div className={mr-2}>
as an inner container as another opportunity to use the existing Boostrap spacing utils referenced in above comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed this in the above comment, but to summarize: we prefer to encapsulate all the styles for separation of concerns and to enhance readability. styles.container
encapsulates whatever style is being applied to the container of the FileLabels. This is because when I'm reading the component logic, I'm not concerned with what classes are being applied, which improves readability.
Also, if in the future we want to add another style property, we won't need to modify the component file, just the .scss file. This is as it should be, since we're only modifying styles. In a way, we're following the open/closed principle, we allow for the extension (adding or changing styles) without needing to modify the actual component file (the logic part)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes the most sense for these applications. I appreciate the explanation and hadn't considered some of the things you've pointed out. I will keep those in mind when developing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitted a few questions, I don't believe they are blockers so you're welcome to ignore since they are mostly syntax things that we don't practice elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems to be good on my end, and thank you again for taking the time to answer my questions!
looks good! Merging |
…ction-to-the-file-page 263 - Add file metadata section to the File page
What this PR does / why we need it:
This PR adds the file metadata section UI of the File Page.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
npm install
.cd packages/design-system && npm run build
.cd ../../
.npm run storybook
.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
File Metadata section added to the File Page
Is there a release notes update needed for this change?:
File Metadata section added to the File Page
Additional documentation: