Skip to content
This repository was archived by the owner on Mar 25, 2021. It is now read-only.

Limit component#119

Merged
guergana merged 3 commits into
limitUIfrom
LimitComponent
Jan 6, 2021
Merged

Limit component#119
guergana merged 3 commits into
limitUIfrom
LimitComponent

Conversation

@guergana
Copy link
Copy Markdown
Contributor

@guergana guergana commented Dec 23, 2020

added limit component.

Still missing some tests.

The validation of the number in the field needs to be discussed as well, also if limit? is required or not and if it can be required conditionally when useLimit == true

@guergana guergana changed the base branch from master to limitUI December 23, 2020 12:52
@guergana guergana force-pushed the LimitComponent branch 2 times, most recently from b67b932 to 82749a7 Compare December 23, 2020 13:02
@guergana guergana changed the title Limit component [WiP] Limit component Dec 23, 2020
@guergana guergana changed the title [WiP] Limit component Limit component Jan 4, 2021
@guergana guergana requested review from Ladsgroup and micgro42 January 4, 2021 12:53
Comment thread src/sparql/QueryRepresentation.ts Outdated
Copy link
Copy Markdown
Collaborator

@micgro42 micgro42 left a comment

Choose a reason for hiding this comment

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

It is a bit strange to add the action and mutation in the same PR as the component, but then not use it there. But since that is done in the following PR, it is not worth splitting this PR up

Comment thread src/components/Limit.vue
name="limit"
v-model="checked"
/>
<label for="limit">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the for attribute refers to an id, not a name, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#attr-for

Copy link
Copy Markdown
Contributor Author

@guergana guergana Jan 5, 2021

Choose a reason for hiding this comment

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

This will be merged in the other PR eventually. This is a mistake yes..

Copy link
Copy Markdown
Contributor Author

@guergana guergana Jan 5, 2021

Choose a reason for hiding this comment

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

This will be replaced, as the label, and even the component are going to be replaced. Will update it but I dont think it's worth spending much time on this part, tomorrow it will be probably decided how to handle this.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If the idea is to merge #120 into this PR first, then I'm ok with it :)

Copy link
Copy Markdown
Contributor Author

@guergana guergana Jan 5, 2021

Choose a reason for hiding this comment

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

Oh, no. 😅 The idea was to merge this one into PR 118. I've updated this PR

@micgro42 micgro42 mentioned this pull request Jan 5, 2021
Copy link
Copy Markdown
Collaborator

@micgro42 micgro42 left a comment

Choose a reason for hiding this comment

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

I think this is now ok by itself, though there is still the question about using messages vs string literals for the copy for now, which is discussed in #118

@guergana
Copy link
Copy Markdown
Contributor Author

guergana commented Jan 6, 2021

I think this is now ok by itself, though there is still the question about using messages vs string literals for the copy for now, which is discussed in #118

Let's ask the designers and then take a decision :)

@guergana guergana merged commit ee8b894 into limitUI Jan 6, 2021
@guergana guergana deleted the LimitComponent branch January 6, 2021 10:50
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
guergana added a commit that referenced this pull request Jan 6, 2021
* added a Limit Component that allows to change limit

* added useLimit action, getter and mutation to decide if limit
number should be included in query

* modified query representation to add limit only if useLimit is true
@guergana guergana mentioned this pull request Jan 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants