Skip to content
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

(fix) O3-3627: Hide filetype extension for image attachment #2071

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

suubi-joshua
Copy link
Contributor

@suubi-joshua suubi-joshua commented Oct 21, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

I am hiding away the filetype extension from being displayed in the UI but is still maintained when the attachment is saved to the server. Also to restrict users from adding duplicate filetype extension I have restricted the user from typing a dot [.] in the filename field.

Screenshots

Before

Screencast.from.22-10-24.00.02.57.webm

After

Screencast.from.21-10-24.23.56.18.webm

Related Issue

https://openmrs.atlassian.net/browse/O3-3627
openmrs/openmrs-esm-core#1183

Other

In this I only was able to hide the filetype extension form user but still one other issue I believe solved by this PR here. The filename input by user is not what is saved and displayed but the default filename. From the attachment videos above you can realize that, the however much I update the filename the default is still maintained which is not desired. This is so because in the filename appended is from the default file object in the uploadedfile type not the filename.

@denniskigen
Copy link
Member

Could you include a bump for the framework as part of your changes now that openmrs/openmrs-esm-core#1183 got checked in?

@suubi-joshua
Copy link
Contributor Author

Thanks @denniskigen let me work on that

@suubi-joshua
Copy link
Contributor Author

suubi-joshua commented Oct 22, 2024

@denniskigen With the changes this is what happens
Screencast from 22-10-24 19:54:53.webm

I blocked out any attempt to of having a dot or comma

package.json Outdated Show resolved Hide resolved
Comment on lines 73 to 74
const fileExtension = uploadedFile.fileName.match(/\.[^/.]+$/)?.[0] || '';
const [fileName, setFileName] = useState(uploadedFile.fileName.replace(/\.[^/.]+$/, '')); // Display name without extension
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const fileExtension = uploadedFile.fileName.match(/\.[^/.]+$/)?.[0] || '';
const [fileName, setFileName] = useState(uploadedFile.fileName.replace(/\.[^/.]+$/, '')); // Display name without extension
const fileExtension = uploadedFile.fileName.match(/\.[^\\/.]+$/)?.[0] || '';
const [fileName, setFileName] = useState(uploadedFile.fileName.replace(/\.[^\\/.]+$/, '')); // Display name without extension

@@ -88,10 +88,13 @@ const MediaUploaderComponent = () => {
</div>
)}
<p className="cds--label-description">
{t('fileUploadSizeConstraints', 'Size limit is {{fileSize}}MB', {
{t('fileUploadSizeConstraints', 'Size limit is {{fileSize}}MB.', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{t('fileUploadSizeConstraints', 'Size limit is {{fileSize}}MB.', {
{t('fileUploadSizeConstraints', 'Size limit is {{fileSize}}MB', {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better not to include punctuation in translation strings and it's not clear to me that the period is useful here. (Keep in mind changing the English text of a translation invalidated every existing translation of this string).

In this case, I would just wrap both clauses in a <p> element so they are on separate lines.

@@ -28,7 +28,7 @@ export function createGalleryEntry(data: AttachmentResponse): Attachment {
return {
id: data.uuid,
src: `${window.openmrsBase}${attachmentUrl}/${data.uuid}/bytes`,
filename: data.filename,
filename: data.filename.replace(/\.[^/.]+$/, ''),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing the extension here?

Copy link
Contributor Author

@suubi-joshua suubi-joshua Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createGallary Function passes the AttachmentResponse data of file details to attachment type which is used by the UI components ie the attachments-overview-component, and the grid and table components. These components all use the filename from the Attachment type and filename comes with the extension.
Before

image

After
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be nice to have a comment explaining things.

@denniskigen denniskigen changed the title (fix) O3-3627 Hide filetype extension for image attachment (fix) O3-3627: Hide filetype extension for image attachment Oct 23, 2024
@suubi-joshua
Copy link
Contributor Author

@denniskigen from earlier discussions I have reverted the logic to restrict the dot and the comma. Also tested out the add image in the visit not and works well the image is added in the attachment gallary.
Screencast from 24-10-24 23:56:11.webm

@suubi-joshua
Copy link
Contributor Author

suubi-joshua commented Oct 25, 2024

@denniskigen and @ibacher In my final iterations I have added a sanitize function which removes any duplicate filetype extensions if typed by user such that the original filetype is maintained as the only one.
Screencast from 25-10-24 12:41:27.webm

@suubi-joshua
Copy link
Contributor Author

suubi-joshua commented Oct 27, 2024

@denniskigen not quite sure why this e2e test is failing for the success notification
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants