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

Should trigger initial filters on first time load and other bugs #1706

Merged
merged 4 commits into from
Sep 25, 2023

Conversation

azmy60
Copy link
Contributor

@azmy60 azmy60 commented Sep 22, 2023

Animation4

Bugs:

  • Does not run initialFilters initially
  • After going to a table from a fk link, try adding some new filters, it will show empty AND/OR buttons
  • Cannot focus the filter input with ctrl+f
  • Converting to raw filter when the filter value is not a string gives an error. Steps to reproduce:
    1. Make a filter that uses number, e.g. id => equals => 1
    2. Disconnect from db and reconnect
    3. Try switching to raw filter

Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

export from a different file & investigate initialFilter

@@ -224,7 +224,7 @@ import data_converter from "../../mixins/data_converter";
import DataMutators, { escapeHtml } from '../../mixins/data_mutators'
import { FkLinkMixin } from '@/mixins/fk_click'
import Statusbar from '../common/StatusBar.vue'
import RowFilterBuilder from './RowFilterBuilder.vue'
import RowFilterBuilder, { normalizeFilters } from './RowFilterBuilder.vue'
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should move this function somewhere else, Vue files should really only contain the vue component. Maybe utils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@@ -924,6 +924,7 @@ export default Vue.extend({
this.rawTableKeys = await this.connection.getTableKeys(this.table.name, this.table.schema)
const rawPrimaryKeys = await this.connection.getPrimaryKeys(this.table.name, this.table.schema);
this.primaryKeys = rawPrimaryKeys.map((key) => key.columnName);
this.filters = normalizeFilters(this.initialFilters || [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we need to do this previously? I don't think we did, shouldn't initialFilter in the tabulator constructor do this?

Copy link
Contributor Author

@azmy60 azmy60 Sep 22, 2023

Choose a reason for hiding this comment

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

Actually, we had this before but in a little different way. this.filter was used in filterForTabulator() which then used in many different parts including the tabulator dataFetch.

In this case, this.filters is empty and was not assigned with the initialFilters initially, which is why when we're fetching the data in dataFetch(), the data is not filtered at all.

Edit:
Btw, I removed the filterForTabulator() so we can keep the logic for building filters just in <RowFilterBuilder />

Copy link
Contributor Author

@azmy60 azmy60 Sep 22, 2023

Choose a reason for hiding this comment

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

I accidentally removed that part and didn't know it's actually gonna affect the fk click. 🤦

@azmy60 azmy60 changed the title Trigger initial filters after clicking fk keys and fix empty AND OR button Should trigger initial filters on first time load and other bugs Sep 22, 2023
Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

Great stuff! Excited to launch this 🚀

@rathboma rathboma merged commit cf5fdb4 into beekeeper-studio:master Sep 25, 2023
4 checks passed
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.

None yet

2 participants