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

feat: Implement UI for create and update #252

Merged
merged 14 commits into from
Oct 25, 2024

Conversation

petesfrench
Copy link
Contributor

Done

  • Creates a new template that will be used for both 'create' and 'update', create-update.html`. The fields are disabled and enabled based on whether they are editable in each state, 'create' and 'update'.
  • When the form is rendered for 'update', the data for the relevant asset is passed to the template and the form is pre-filled with existing asset information. This is accomplished partly using Jinja and partly with JS

QA

  • Check out this feature branch and run the project
  • Navigate to the 'Add asset' tab (http://0.0.0.0:8017/manager/create) and attempt to upload an asset, making sure to test each field. It would be a good idea to test with different combinations of fields and, when relevant, test with different amounts of inputs ie. on tags.
  • Navigate to the asset update page, (http://0.0.0.0:8017/manager/update?file-path=FILE_PATH), and see that the fields you previously selected/input are already filled in. Update some fields and click update, see you are redirected to the same page but with the relevant fields updated.
  • Make sure it matches the design/fields/descriptions ect in the figma and spec

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-13751

@immortalcodes
Copy link
Member

Are we covering all the languages that are required here?

image

Also if there is just one langauge could we say "select the language which applies".

@immortalcodes
Copy link
Member

immortalcodes commented Oct 23, 2024

Also if I specifically not press enter after every tag, it will take the entire tag array as a single tag
image
This is true for UI atleast
Uploading image.png…

Edit:This is also true for backend, In my opinion tags should be single worded with use of - or _ for separation

@immortalcodes
Copy link
Member

If I am right according to specs only non-image assets could have name?

@immortalcodes
Copy link
Member

immortalcodes commented Oct 23, 2024

Base64 encoding failed here
image

I am not sure but should we output error to API response? This is definitely helpful for devs but I am not sure if it's the best practice.

@immortalcodes
Copy link
Member

image
For some reason tags didn't went through and I think it is because I just typed them and didn't press enter.
Please ignore this if this is supposed to be the design

@immortalcodes
Copy link
Member

immortalcodes commented Oct 23, 2024

Update asset should take me to the home screen but I guess keeping it to same screen is intended here.
image

@petesfrench
Copy link
Contributor Author

petesfrench commented Oct 23, 2024

Are we covering all the languages that are required here?

image

Also if there is just one langauge could we say "select the language which applies".

Definitely not covering all language here. If we want to cover all language we will have to implement something a little more advanced to make it easier to find the appropriate language. But given that this is an MVP, this is fine for now.

In the figma it is a multiselect. Why? I do not know haha

Edit: I just checked the spec, they actually include a smaller list of languages. But again, this is mvp, so is likely fine

@petesfrench
Copy link
Contributor Author

@immortalcodes

For the tags section, I am not sure the proper UX pattern follow here. The figma said to use search and filter, but there is nothing to search so I tried to adapt it. I will ask @Sophie-32 tomorrow.

For the non-image asset not having names, where can you see this in the spec? I don't see why we would do that.

For failed base64 encoding, I have hopefully fixed this in the next PR

For the second tags comments, this again related to the UX of how it should work. Will address tomorrow with Sophie's input.

For redirecting after editing, yes this is temporary until we have a 'details' page. Which is being added in the next PR.

@immortalcodes
Copy link
Member

@immortalcodes

For the tags section, I am not sure the proper UX pattern follow here. The figma said to use search and filter, but there is nothing to search so I tried to adapt it. I will ask @Sophie-32 tomorrow.

For the non-image asset not having names, where can you see this in the spec? I don't see why we would do that.

For failed base64 encoding, I have hopefully fixed this in the next PR

For the second tags comments, this again related to the UX of how it should work. Will address tomorrow with Sophie's input.

For redirecting after editing, yes this is temporary until we have a 'details' page. Which is being added in the next PR.

Makes sense, for the name thing here is the comment that says add name if it's a PDF. Not sure if this is valid for all.
https://www.figma.com/design/dOz7GIHMmofSdje0l0pVFs?node-id=12-4914#945668040

@immortalcodes
Copy link
Member

immortalcodes commented Oct 24, 2024

There are few things that are still in discussion and might need correction later(with more clarity from design and UX), We could also plan a bit of refactoring in later stages, but this is really good work and checks everything for MVP.
Cheers!
Just take care of checks and it should all be good.

@petesfrench petesfrench merged commit ad099cd into feature_assets_improvement Oct 25, 2024
5 checks passed
@petesfrench petesfrench deleted the wd-13751 branch October 25, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants