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

Features/863 move project description #1121

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented Jan 8, 2025

Closes getodk/central#863

What has been done to verify that this works as intended?

Manually tested it. The visual aspect of this features needs to be thoroughly tested by QA team.

Why is this the best possible solution? Were any other approaches considered?

For now all the calculations are kept inside description component, if need be those can be extracted out to utility module or custom vue directive.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

None

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

NA

Before submitting this PR, please make sure you have:

  • run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

src/components/project/overview/description.vue Outdated Show resolved Hide resolved
src/components/project/overview/description.vue Outdated Show resolved Hide resolved
src/components/project/overview/description.vue Outdated Show resolved Hide resolved
src/components/project/overview/description.vue Outdated Show resolved Hide resolved
src/components/project/overview/description.vue Outdated Show resolved Hide resolved
and couple of tests for collapsed view of description
@sadiqkhoja sadiqkhoja marked this pull request as ready for review January 13, 2025 18:10
Copy link
Member

@matthew-white matthew-white left a comment

Choose a reason for hiding this comment

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

Looks great 😎

@@ -23,7 +23,7 @@ describe('EntityUploadDataTemplate', () => {
content.should.equal('label,hauteur,circonférence');
});

it('has the correct filename', async () => {
it.skip('has the correct filename', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to revert this before merging.

onMounted(() => {
// nextTick is good enough for the calculation being done here. I don't see
// any screen flicker. Maybe related: https://github.com/vuejs/vue/issues/9200.
// Additionally Without nextTick offsetHeight is always 0.
Copy link
Member

Choose a reason for hiding this comment

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

I bet this is because there's a v-show on <page-head>. Normally I wouldn't expect nextTick() to be needed in onMounted(), since the element is in the DOM at that point. In any case, I think it makes sense the way it is. 👍

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.

Update Project description
2 participants